Material Design for the CreateGistActivity #908

Merged
merged 14 commits into from Jan 3, 2017

Projects

None yet

5 participants

@larsgrefer
Contributor
  • using the design library as mentioned in #818 including:
    • TextInputLayout
    • CoordinatorLayout
    • FloatingActionButton

screenshot_20151009-231149

@StefMa
Contributor
StefMa commented Oct 10, 2015

Haven't yet see the code. But the image look great.
But I should don't using the FAB instead of menu because of user experience.
If the user type something, then the keyboard pops up and hide the fab. So the user need always close the keyboard to create a gist. That's bad, in my opinion.
I would stay on the menu item.

@larsgrefer
Contributor

@StefMa The FAB is keyboard-aware

screenshot_20151010-152651

@StefMa
Contributor
StefMa commented Oct 10, 2015

And now it hides the content, when content is match parent :-)

@Meisolsson
Contributor

Even if it follows the keyboard I still prefer to have the save/done button in the actionbar.

@fadils
Member
fadils commented Oct 10, 2015

Agrees with @Meisolsson. Prefer save/done.

When we are faced with a list of something and want to create a new item. Use FAB.
During the creation of that very item. Use save, done, tick, etc. But not FAB.

My 2 cents.

screenshot_2015-10-10-22-16-50
screenshot_2015-10-10-22-16-20

@larsgrefer
Contributor

@StefMa @Meisolsson @fadils You're all right. I've updated the PR

@hzsweers
Contributor

The actual gist area should be more distinguished, and make it obvious that this is somewhere to either paste or edit large amounts of text/code. Rather than a text input that starts with one line, I think it should be different background color (either make the content above it have a darker grey with this white or something similar), full width, and take up the remaining vertical space below the description and file name. Thoughts? @Meisolsson @fadils

@Meisolsson
Contributor

I think making the background white and make it take up all the vertical space that's left is the best approach. Full-width sound weird but i think it might look good.

@hzsweers
Contributor

yeah, just to make it obvious that this is like, a "sheet" if you will where you can put your gist data and have it clearly seprated as content from the metadata above it

@larsgrefer
Contributor

We could put all the other inputs into an collapsible actionbar like shown below and leave the main content area for only the gist's content Example Picture

@StefMa
Contributor
StefMa commented Oct 10, 2015

The last screen looks very great in my opinion. I also think we don't need a "full width" text input. A single line is enough. But we need to separated it. And this is in the last screen very great.

But there is another point. On Web you can make one Gist with multiple files and titles. What is with that? Currently we support only one Gist with one title and one file...

@larsgrefer
Contributor

@StefMa I agree with you, we should allow creating gists with multiple files, but the purpose of this PR is only to implement material design for the existing functionality

@StefMa
Contributor
StefMa commented Oct 10, 2015

Then the last screen is the best solution ;-)

@larsgrefer
Contributor

@hzsweers @Meisolsson @StefMa Any opinions about this version?

The separation between metadata and content should be strong enough now.

screenshot_20151011-150732
screenshot_20151011-150737

@StefMa
Contributor
StefMa commented Oct 11, 2015

Yes this is great! Put only some elevation on the toolbar and that's all.
Is the margin left/start designed like the guidelines? It looks too me to wide...

@larsgrefer
Contributor

56 dp marign + 16 dp padding = 72dp
Its the same margin as used in the sample image above and as (often) mentioned here:

Content left margin from screen edge: 72dp

(56dp = ?attr/actionBarSize)

@larsgrefer
Contributor

@StefMa What about just dropping the gist-icon and adjust the rest a bit

In fact actionbar icons are pretty uncommon in material design. In my opinion such icons only make sense when showing an user or organization avatar.

screenshot_20151011-165218

@StefMa
Contributor
StefMa commented Oct 11, 2015

Oh yeah, that was the eye catcher. This looks nicer now.
I think we can merge it ;)

@larsgrefer
Contributor

Wait until i updated the PR in order to reflect the screenshot

@larsgrefer
Contributor

btw: someone should add the labels material and design to this, right?

@larsgrefer
Contributor

@StefMa in fact there is an elevation of 4dp on the AppBarLayout defined in Widget.Design.AppBarLayout:

<style name="Widget.Design.AppBarLayout" parent="android:Widget">
        <item name="elevation">@dimen/design_appbar_elevation</item>
        <item name="android:background">?attr/colorPrimary</item>
</style>

The elevation is loaded correctly in the constructor of AppBarLayout.
I have no idea why its not drawn or not visible.

@hzsweers
Contributor

Left align the header fields for this and I'll merge it. I know it's currently aligned with the actionbar title, but the extra space just looks weird. MD suggest that 72dp left margin assuming there's an icon or something there, which there isn't in this case.

@larsgrefer
Contributor

@hzsweers I've rebased everything and removed the left margin

@larsgrefer
Contributor

Travis fails because of missing build-tools, see #970 to fix this

@hzsweers
Contributor

Can you upload a screenshot?

@larsgrefer
Contributor

@hzsweers Sure

screenshot_20151228-121607

@hzsweers
Contributor

Few more nitpicks:

  • Can we keep white focused color rather than the blue? Blue on dark grey doesn't look that great to me
  • Cap sentences for the description
  • Let's limit the description to just 3 lines and scroll after that
  • File name should have inputType of textUri or just text. No suggestions, swiping, and putting a period (such as with a file extension) shouldn't automatically insert a space like it's a sentence
  • The toolbar menu item should be greyed out until it's clickable, not obvious in its current state that it's disabled.
  • Can we add an elevation to the header so there's a minor dropshadow on the content? Would add a little bit of differentiation.
  • Can we push the file name into the toolbar title when collapsed? Fine if not, just an idea
  • Can clicking anywhere in the header or its input fields have it auto-expand to its full height? Otherwise you could edit partially-cutoff input fields :/
@Meisolsson
Contributor

Could we move the checkbox to go under the filename, looks kind of weird to have it at the top

@larsgrefer
Contributor

Sure we can move the checkbox. Should we put it between the text inputs or below both inputs?
Am 29.12.2015 1:00 nachm. schrieb Henrik Olsson notifications@github.com:Could we move the check box to go under the filename, looks kind of weird to have it at the top

—Reply to this email directly or view it on GitHub.

@Meisolsson
Contributor

Try it below both

@larsgrefer
Contributor

@hzsweers

Can we keep white focused color rather than the blue? Blue on dark grey doesn't look that great to me

Done

Cap sentences for the description

Done

Let's limit the description to just 3 lines and scroll after that

Done

File name should have inputType of textUri or just text. No suggestions, swiping, and putting a period (such as with a file extension) shouldn't automatically insert a space like it's a sentence

Done

The toolbar menu item should be greyed out until it's clickable, not obvious in its current state that it's disabled.

I agree, which way would you suggest to implement this?

Can we add an elevation to the header so there's a minor dropshadow on the content? Would add a little bit of differentiation.

In fact the elevation is there, but its somehow not drawn: #908 (comment)

Can we push the file name into the toolbar title when collapsed? Fine if not, just an idea

Not sure how to do this properly

Can clicking anywhere in the header or its input fields have it auto-expand to its full height? Otherwise you could edit partially-cutoff input fields :/

Done.
The AppBar expands itself if anything in it recieves focus or changes its value

@Meisolsson

Try it below both

Done

@hzsweers
Contributor

In fact the elevation is there, but its somehow not drawn

Try setting the parent layout to not clip children, or manually set an outline provider on it (like so https://github.com/Flipboard/bottomsheet/wiki/Recipes#shadow-and-elevation)

@larsgrefer
Contributor

@hzsweers I've tried both options. None of them seem to work

@hzsweers
Contributor

For the toolbar icon, implementation wise we can set a color filter or alpha on the icon's drawable

@StefMa
Contributor
StefMa commented Oct 1, 2016

Ok, One year later :D
Why isn't this finally merged? The UI looks awesome 👍
Maybe @larsgrefer can fix the conflicts and we can get this into the app?!

@Meisolsson
Contributor

The only small thing here is the disabled color of the toolbar menu item. If you can't figure it out just fix the merge conflicts and I'll fix it later. When it's ready to merge I'll review the code.

@Meisolsson
Contributor

Sorry, didn't see the changes... LGTM!

@Meisolsson Meisolsson merged commit c1078df into pockethub:master Jan 3, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment