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

Material FAB with speed dial implementation #1100

Merged
merged 43 commits into from
Mar 9, 2016
Merged

Material FAB with speed dial implementation #1100

merged 43 commits into from
Mar 9, 2016

Conversation

AndyScherzinger
Copy link
Contributor

This PR is for discussion and experimentally test driving a FAB implementation.

Nothing to see yet but I wanted to open a PR in order to check for build compliance and discuss the progress and changes/adaption of any feedback. Incorporating the discussion on #983

The branch is based on latest master as in release 1.9.1

@AndyScherzinger
Copy link
Contributor Author

So here we go... Initial implementation of the Speed Dial FAB :) pinging @tobiasKaminsky @davivel and @jancborchardt

The implementation works for ICS and up, is implemented as a Speed Dial (right now with labels since we do not have good icons to distinguish upload and upload from apps; they can just be taken out in the layout xml 😃 ), animation (mini fabs popping up, +/x animation) and it is linked thus the actions work already.

Open Issues:

  • Action Bar icons left for now (since I don't now when they are still needed since the FAB might not be displayed)
  • The FAB is hovering even in expanded mode but not blocking any other elements, thus you can expand the FAB Speed Dial and still click on list items (unsure if this is an expected behavior, or not though)
  • ANT build is broken since the classpath doesn't get build up correctly atm (unsure why, as always works fine with gradle)... not saying supporting ant builds is frustrating 😉 Any idea what to do here @davivel ?

And here are the screenshots...

Lollipop
device-2015-08-18-001613
device-2015-08-18-001624

ICS
device-2015-08-18-001749
device-2015-08-18-001859

@AndyScherzinger
Copy link
Contributor Author

...nevermind, found the classpath problem 🎉 🎉 🎉

@AndyScherzinger AndyScherzinger mentioned this pull request Aug 20, 2015
@AndyScherzinger AndyScherzinger changed the title Material FAB implementation Material FAB with speed dial implementation Aug 20, 2015
@AndyScherzinger
Copy link
Contributor Author

@jancborchardt can you provide an icon for the "upload from app" button. I looked through the Google provided icons but couldn't find one that would have expressed the action at all :(

@jancborchardt
Copy link
Member

Hmmm … maybe https://www.google.com/design/icons/#ic_content_copy or https://www.google.com/design/icons/#ic_add ?

But I’d also be fine with having both be the same icons.

@tobiasKaminsky
Copy link
Contributor

Both actions the same icon is not so good as I do not like the text and thought, that the text will be removed.
If there is no suitable icon, maybe just open a choice dialog as it is now? And only have a "+" icon for both actions.
Although I would really like to have two meaningful icons...

@AndyScherzinger
Copy link
Contributor Author

I agree with @tobiasKaminsky that using one icon for two to be distinguished actions is rather confusing for the user. I put in the labels for now to be able to distinguish the actions but would also not want to go live with the labels.

As for the choice dialog that seems not quite right but probably a pragmatic solution for now and imho we should really aim for two fitting icons since that would lead to a proper use of the speed dial and wouldn't increase the number of click (still two as of today) instead of three if we would use a choice dialog.

The suggested icons don't seem to find in my opinion since the copy icon is used in a context where the user copies a selection on the same screen which wouldn't be the case here and thus an unexpected behavior, as for the plus icon that is already used as the icon om the FAB and thus shouldn't have two different actions triggered within the same context. I searched through https://materialdesignicons.com/ but couldn't find an appropriate icon for the app-upload. If someone has a nice icon which is related to "app" icon can create a derivation for app-upload :)

@joeplus
Copy link

joeplus commented Aug 21, 2015

What about this one, rotated by 90° to left so that the arrow points to the top?

@AndyScherzinger
Copy link
Contributor Author

@jancborchardt @tobiasKaminsky what do you think? Not too bad, eyh? :)

@joeplus
Copy link

joeplus commented Aug 21, 2015

Or flip the arrow as well like this

Flipped arrow
upload-from-app 2

Flipped and recessed arrow
upload-from-app

@AndyScherzinger
Copy link
Contributor Author

I kind of prefer the recessed one but would shorten the arrow at the bottom be be more in balance

@tobiasKaminsky
Copy link
Contributor

I have found this:
import
https://materialdesignicons.com/icon/import

But I would like to have a generic app icon and a sending/import arrow.
I think that the user then would better understand it.

And when the text is gone, the text should appear if the user long presses the button. So the user can figure out what the button means instead of just clicking it.

@AndyScherzinger
Copy link
Contributor Author

Alternatively like a learning UI we could show the labels like in the screenshot for the first 1-3 times and after that don't show them anymore implying the user now know the function behind each mini fab.

what do you think?

@jancborchardt any comment on the icon suggestions? :)

@tobiasKaminsky
Copy link
Contributor

Learning UI is great, but still the text should appear when long-pressing (like in k-9)

@AndyScherzinger
Copy link
Contributor Author

@jancborchardt @tobiasKaminsky @joeplus any decision on the icons to be used?

@tobiasKaminsky where should the long click be implemented? on the mini fabs? And should that still be implemented with distinguishable icons?

@AndyScherzinger
Copy link
Contributor Author

Just added the implementation for the click events and cleaned up the action bar menu which now shows the remaining to actions :)

  • click on the min fab gives you the according action
  • long click on the mini fab shows a Toast naming the action behind the mini fab

--> for now I moved to the import icon hoping to jump start the icon discussion and getting to a point where we decide on the icons :)

device-2015-09-11-150252

@jancborchardt
Copy link
Member

Yeah, good! I guess at least for the first time opening the menu, the titles of the icons should be shown. Is that possible?

@AndyScherzinger
Copy link
Contributor Author

@jancborchardt absolutely, that is an easy one basically adding a preference value (under the hood, not in the preference screen) to store the fact, that the labels have been shown one time and if the preference has the value "already shown" the labels won't be added to the UI :) - easy.

That was what I meant with the learning UI.

@AndyScherzinger
Copy link
Contributor Author

any wishes for the coloring of the labels? I can change it to any colors you like 👍 ..probably one of the dark blues or some greyish thing so people don't think they can click on it 😸

@jancborchardt
Copy link
Member

Cool! Yeah, just go with a default half-transparent grey or so, like what you had in a few screenshots above.

@AndyScherzinger
Copy link
Contributor Author

Done!

It'll show labels until the user click on one of the smaller floating buttons and for the first session of the application.
The reason is that I couldn't find a way to remove the labels and resize the buttons click area, thus a transparent space stays until the next start of the application (start, not a pause/rusume!).

So whenever the list fragment gets initialized it checks if a fab ha ever been clicked before und if so doesn't add any labels.

It is not the best solution UX wise, but technically the one that works best for now imho.

@AndyScherzinger
Copy link
Contributor Author

How should we progress here?

Two questions are open imho, directing them to @jancborchardt:

  • should we use the icon chosen for now?
  • is the behavior regarding the showing/hiding of the labels okay?

If the icon and the behavior are okay I can finalize the PR adding the icon work in all the necessary resolutions, so maybe we can than have the two open material design PRs in 1.8.1 😀

Disclaimer just mentioning it again: This PR is based on #1090 so that one should be merged first (material buttons/checkboxes/paddings/margins)

@jancborchardt
Copy link
Member

@AndyScherzinger yeah, I’m fine with the icon and the labels showing on first use. Thought my previous comment was already confirmation for that. :)

@AndyScherzinger
Copy link
Contributor Author

Cool! Well, the first use is not very specific so I asked to get a very specific answer, so for now for the labels, first use basically means first session (in a business manor) and first activity life cycle (technically speaking)

@AndyScherzinger
Copy link
Contributor Author

@davivel Thanks for the code review, changes have been commited or I added comments for further clarification 👍

);
//startActivityForResult(action, ACTION_SELECT_MULTIPLE_FILES);
// this flow seems broken;
getActivity().startActivityForResult(
Copy link
Contributor

Choose a reason for hiding this comment

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

FileDiplayActivity also exposes a public method to do this , would prefer it's used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in the other comment, this is now exclusively handled by the fragment, since the activity doesn't trigger these actions anymore, I slightly refactored the code

  • so I removed the method in the activity 8d038e8
  • moved the requestCode to the fragment f34a9dd
  • also removed the "other" action implementations since they are now handeled by the fab implementation in the fragment 7e82baa
  • shortened the upload call by creating a static helper method within the upload activity 7e82baa

@davivel
Copy link
Contributor

davivel commented Mar 3, 2016

Thanks, @AndyScherzinger . Commented a bit more :$

@AndyScherzinger
Copy link
Contributor Author

Thanks for the thorough code review, I really think this pushed the PRs quality 👍
Please check my comments since I went for a slightly different solution in comparison to your comments. Hope this is fine with you 😃

@AndyScherzinger
Copy link
Contributor Author

@davivel forgot to mention that if there is anything else regarding the PR and its implementatation please let me know! 😃

@davivel
Copy link
Contributor

davivel commented Mar 4, 2016

I like what you did :) .

This is 👍 from dev POV, time for testing.

Thank you so much, pal.

@davivel
Copy link
Contributor

davivel commented Mar 4, 2016

@AndyScherzinger , maybe you want to have a look to the test plan in owncloud/QA#161 . If find anything wrong, or miss something, feel free to comment :)

@AndyScherzinger
Copy link
Contributor Author

Glad I can contribute to owncloud and again thank you very much for your input and the time you spend on my PRs. I am aware that you have a lot of tasks in the oC ecosystem, customer specific tasks, etc. and I am thankful that you still make time to support community contributions!

I'll check the QA task, pleasure to help!
Edit: DONE, I added some test cases that cover functionality I introduced for the FAB

@davivel
Copy link
Contributor

davivel commented Mar 8, 2016

@AndyScherzinger , I modified the color set for normal state of FAB so that it's the same as for 'primary buttons'.

Consider this a temporary fix to complete test pass. We will review the needs of customizable colours soon, and hopefully define clear criteria to apply them in the future. I will keep everybody, but specially you, informed about it.

cc @jesmrec , ready for final test.

@AndyScherzinger
Copy link
Contributor Author

@davivel yeah, perfectly fine with me :) 👍
@jesmrec woohoo! QA approved! This is awesome! 👍

@davivel
Copy link
Contributor

davivel commented Mar 9, 2016

Then everything is OK. Pa' la saca. :)

davivel added a commit that referenced this pull request Mar 9, 2016
Material FAB with speed dial implementation
@davivel davivel merged commit b2c81d3 into master Mar 9, 2016
@davivel davivel deleted the material_fab branch March 9, 2016 09:44
@AndyScherzinger
Copy link
Contributor Author

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉
WOOHOO
🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

@supportreq
Copy link

@AndyScherzinger one query, any particular reason for the FAB labels to be disabled post first restart of the app? I got confused as why the label is missing after restart.. can we have it as a setup.xml preference..

@AndyScherzinger
Copy link
Contributor Author

AndyScherzinger commented May 18, 2016

sure thing, that would then have to be discussed with @davivel and @jancborchardt
In general you can still long-press the mini fabs which will give you the label as a toast.

Edit: The reason for hiding it was that we wanted to create a "self-learing" UI as such that the user will be hinted to the fuctionality via the labels but from then onward we would not show it again since it doesn't add any(more) value.

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.