Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1413 share view enhancement - REDONE #1551

Merged
merged 14 commits into from
Mar 22, 2016
Merged

Conversation

AndyScherzinger
Copy link
Contributor

For details and discussion please see #1413 and original PR #1416

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @davivel and @malkomich to be potential reviewers

@AndyScherzinger
Copy link
Contributor Author

Screenshots...
device-2016-03-21-203546
device-2016-03-21-203559
device-2016-03-21-203617

@AndyScherzinger
Copy link
Contributor Author

@jancborchardt one thing, the user list with 1.9.2 will have three kinds of item types, users, groups and federated shares. For federated I use the "user" icon at the moment, so we are in need for an icon 😄

@jancborchardt
Copy link
Member

@AndyScherzinger it’s fine to use the same icon for federated shares, since we want to make the difference as little as possible to have a similarly seamless UX. In the list display, the username should be displayed like tobias@… (with the server really being dotted out, like in the web app).

@AndyScherzinger
Copy link
Contributor Author

Do you have a screenshot for the @... for user and group to see the difference?

@AndyScherzinger
Copy link
Contributor Author

@davivel is there a test account somewhere that I could use to the the federated shares in action and do the possible UI changes?

@davivel
Copy link
Contributor

davivel commented Mar 22, 2016

@AndyScherzinger , thanks a lot for redoing this one.

is there a test account somewhere that I could use to the the federated shares

That's going to be difficult for the short term. And we are interested in this for a very, very, very short term.

In the list display, the username should be displayed like tobias@… (with the server really being dotted out, like in the web app).

@jancborchardt , we made the list following the web UI, but I can't remember any .... What OC server includes that?

@AndyScherzinger
Copy link
Contributor Author

@davivel you are welcome! No worries, I just wanted to see the user/group/federated display in the web app to better understand Jan's requirement here :)

@AndyScherzinger AndyScherzinger added this to the 1.9.2-current milestone Mar 22, 2016
@@ -27,18 +27,40 @@
<LinearLayout android:orientation="vertical"
android:layout_width="match_parent"
android:layout_height="wrap_content">
<TextView
android:id="@+id/editShareTitle"
android:layout_width="fill_parent"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be match_parent, fill_parent is old wording, long time deprecated (though harmless)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed along with some alignment optimizations 968373a

@davivel
Copy link
Contributor

davivel commented Mar 22, 2016

I can upload some screenshots :)

@davivel
Copy link
Contributor

davivel commented Mar 22, 2016

sharing

shared

Captures of web UI while sharing and after shared. I guess the ... talked about are those at the end of user when there is not space enough in a single line (see second picture).

@davivel
Copy link
Contributor

davivel commented Mar 22, 2016

Code is 👍

@AndyScherzinger , this is your chance to make a very easy rebase ;-)

git pull
git checkout 1413_share_view_enhancement_redo
git rebase master

@davivel
Copy link
Contributor

davivel commented Mar 22, 2016

cc @jesmrec , ready to test.

@AndyScherzinger
Copy link
Contributor Author

😞

git rebase master ends up with:

fatal: Needed a single revision
invalid upstream master

@davivel
Copy link
Contributor

davivel commented Mar 22, 2016

Do you have local copy of master?

Try git checkout master, then back to git checkout 1413_share_view_enhancement_redo

@AndyScherzinger
Copy link
Contributor Author

Hmmmm, I don't get it... Tried what you said, see commit history, again all commits doubled...

We should stop trying to make rebases work for now imho, with the amount of commits this is just to risky. If everything goes according to plan we might be able to teach me this stuff at the next conference... 😞

@AndyScherzinger
Copy link
Contributor Author

@jancborchardt looking at the screenshots this might be a really tricky one since we don't have that much space on a cellphone, right?
device-2016-03-21-203546
device-2016-03-21-203559

@AndyScherzinger
Copy link
Contributor Author

Else we might be able to do two lines and add that to a second line - ?

@davivel
Copy link
Contributor

davivel commented Mar 22, 2016

Two lines not, please...

Let me see how it's now, I'll push a screenshot with a federated share.

@AndyScherzinger
Copy link
Contributor Author

I'm fine with it the way it is, question is, if we pack in the server address + port it'll be very packed. That is just my concern here.

@davivel davivel force-pushed the 1413_share_view_enhancement_redo branch from cab6f19 to 5feba6a Compare March 22, 2016 12:29
@davivel
Copy link
Contributor

davivel commented Mar 22, 2016

Hmmmm, I don't get it... Tried what you said, see commit history, again all commits doubled...

Don't worry, I rebased. This was not so tricky, since there was no conflict.

@jancborchardt , screenshot of federated share:
device-2016-03-22-133737

Ellipsis is done in the middle, a common practice in our mobile apps, if I recall correctly. Is it OK, or should we ellipsize in the end ?

@AndyScherzinger
Copy link
Contributor Author

Looks good to me 👍
Personally I would stick with the ellipsis in the middle like @davivel said this common practice in the mobile app and ellipsizing at the end but still having the port definition makes things a bit complicated just for the display of the more or less irrelevant information from an end user's perspective (port number).

@AndyScherzinger
Copy link
Contributor Author

@carlaschroder just in case this is relevant for documentation, please see above the latest screenshots of the share dialog(s) for the upcoming release 1.9.2 😃

@javiergonzper
Copy link
Contributor

QA approved 👍

@davivel
Copy link
Contributor

davivel commented Mar 22, 2016

Then... in!

davivel added a commit that referenced this pull request Mar 22, 2016
@davivel davivel merged commit 5edc4a1 into master Mar 22, 2016
@davivel davivel deleted the 1413_share_view_enhancement_redo branch March 22, 2016 17:08
@AndyScherzinger
Copy link
Contributor Author

AWESOME! 🎉

@jancborchardt
Copy link
Member

Nice, very cool! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants