[docs] Add demo for a bottom app bar #13030

adeelibr · 2018-09-28T17:00:11Z

Added a demo in the app-bar section for bottom app-bar.

Related to issue:
#12956 (comment)

Edit from @eps1lon:
preview of the created demo from this PR

adeelibr · 2018-10-02T14:59:33Z

@oliviertassinari can you please tell me, the improvement/changes that I need to do for this? :)

oliviertassinari · 2018-10-02T15:32:20Z

@adeelibr It comes from my understanding that the pull request isn't finished yet.

adeelibr · 2018-10-02T18:25:14Z

Can you kindly provide me with some feedback, on the missing stuff from this PR. So that I can start working on it right away. I thought I was done 😅

oliviertassinari · 2018-10-02T18:45:05Z

Form my understanding the main pain point to solve is around scroll handling, both for top and bottom app bar. We could also have a demo with icons, closer to what the specification demonstrate.

mbrookes · 2018-10-02T19:17:14Z

You could try following this example: https://material.io/design/components/app-bars-bottom.html#anatomy

Centered FAB seems to be the defacto (I'm aware we need fix the default color...).

mbrookes · 2018-10-02T19:08:13Z

docs/src/pages/demos/app-bar/app-bar.md

@@ -38,3 +38,7 @@ A primary searchbar.
 ## Dense (desktop only)

 {{"demo": "pages/demos/app-bar/DenseAppBar.js"}}
+
+## Appbar Fixed Bottom


"Bottom App Bar"?

mbrookes · 2018-10-02T19:17:45Z

docs/src/pages/demos/app-bar/BottomAppBar.js

+  return (
+    <div className={classes.container}>
+      <section className={classes.main}>
+        <p>


Typography (Although I'm not mad on the wall of text for this demo.)

Actually, according to the spec, it isn't even valid usage: https://material.io/design/components/app-bars-bottom.html#usage

Understood! Working on it! I am sorry for the late reply, I have been stuck with office work! I'll have it done by tomorrow. Thank you for your guidance.

adeelibr · 2018-10-04T19:41:23Z

Is this better @mbrookes @oliviertassinari ? I tried replicating the example from material design

mbrookes · 2018-10-04T21:41:28Z

@adeelibr Better, but based on the text and choice of icons, that looks like a top app bar moved to the bottom. Better to follow the example from the spec as requested.

Also, as mentioned, thebottom app bar isn't valid usage in conjunction with a page of text:

Actually, according to the spec, it isn't even valid usage: https://material.io/design/components/app-bars-bottom.html#usage"

adeelibr · 2018-10-07T07:48:03Z

@mbrookes did you mean something like this?

and on mobile;

adeelibr · 2018-10-07T07:51:11Z

Also while I was looking at the link you shared above, one link let to the other 😄 and I found an issue created on the material-components material-components/material-components-web#2659 Some one made an example with the AppBar with fab icon in center cut http://jsfiddle.net/Toufic/czpb8u0f/5/

Would it be okay to add this example using our components in the Appbar documentation, I'd love to create another PR after this is done, .. whenever this is done. 😅

mbrookes · 2018-10-07T10:47:10Z

Much better. You could lose the "Material-UI" text (that doesn't belong on a bottom AppBar), and set a width on the demo. You could follow: https://material-ui.com/demos/buttons/#floating-action-buttons (width: 500).

The FAB doesn't appear to stay centered as you resize.

The "Inbox" label at the top of the list doesn't look right to me. Do you have a an example of that from the spec? You could make the subheader sticky though.

Regarding the MCW inset example, it's a clever hack, but it doesn't meet the spec in a couple of important ways. The cutout is solid white rather than transparent, so content scrolled behind would get cut off. The corners of the cutout aren't rounded. IT doesn't support scrolling the app-bar out of view.

Interesting to see that MCW uses CSS variables. (No IE11 or Opera Mini support.)

adeelibr · 2018-10-07T12:46:58Z

The "Inbox" label at the top of the list doesn't look right to me. Do you have a an example of that from the spec? You could make the subheader sticky though.

I took the example from https://material.io/design/components/app-bars-bottom.html#behavior

I have made the subheader sticky.

Regarding the MCW inset example, it's a clever hack, but it doesn't meet the spec in a couple of important ways. The cutout is solid white rather than transparent, so content scrolled behind would get cut off. The corners of the cutout aren't rounded. IT doesn't support scrolling the app-bar out of view.

Interesting to see that MCW uses CSS variables. (No IE11 or Opera Mini support.)

The examples where not official, it was made by a contributor. But MCW hasn't added it into their official docs.

adeelibr · 2018-10-07T17:29:59Z

Also regarding the implementation I showed earlier, I played with it a bit, and I can see the content behind the fab button. As you can below. Your thoughts?

regarding the part

IT doesn't support scrolling the app-bar out of view.

I think we can play with this using vanilla js specifically the scroll event listener. window.addEventListener("scroll", () => {});

mbrookes · 2018-10-07T19:12:08Z

I can see the content behind the fab button

Interesting. There appeared to be a solid white bar behind the purple bar. I admit I didn't try creating scrollable content.

mbrookes · 2018-10-07T20:21:08Z

There appeared to be a solid white bar behind the purple bar.

Okay, no, it's actually a transparent bar with a drop-shadow, it just looked white due to the white background:

This does mean that there's a shadow where there shouldn't be, overlaying the content behind, but nothing you can do about that with pure CSS.

It doesn't support scrolling the app-bar out of view.

I think we can play with this using vanilla js specifically the scroll event listener. window.addEventListener("scroll", () => {});

No, that isn't the problem: since the "cut-out" is a transparent div with a border-radius, there's no way to change the shape to reflect the portion of the FAB that intersects with it as it animates out.

It's a decent compromise for a fixed app bar with inset FAB, but the whole point of the inset FAB seems to be to support transitions and scrolling, so fixed doesn't make much sense.

adeelibr · 2018-10-08T05:06:25Z

Understood.

Regarding this current PR, is this PR okay to be merged?

mbrookes · 2018-10-08T09:31:24Z

Nearly. The sticky subheader shouldn’t have a transparent background, and now that the demo is in better shape, it also needs code review.

adeelibr · 2018-10-08T17:35:34Z

Done sir. Can you please review the code. @mbrookes

mbrookes

Okay, this is starting to look really good! A couple of typos in the messages, and a couple of nice-to-haves, and we're there!

mbrookes · 2018-10-08T17:47:00Z

docs/src/pages/demos/app-bar/BottomAppBar.js

+  {
+    id: 3,
+    primary: 'Recipe to try',
+    secondary: 'I am try out this new BaqBQ recipe, I think this might be amazing',


-new BaqBQ + new BBQ

mbrookes · 2018-10-08T17:49:12Z

docs/src/pages/demos/app-bar/BottomAppBar.js

+  },
+  {
+    id: 5,
+    primary: 'Doctors Appointment',


-Doctors +Doctor's

mbrookes · 2018-10-08T17:49:32Z

docs/src/pages/demos/app-bar/BottomAppBar.js

+  {
+    id: 5,
+    primary: 'Doctors Appointment',
+    secondary: 'My appointment for the dentist was rescheduled for next Saturday.',


-dentist +doctor

mbrookes · 2018-10-08T17:59:37Z

docs/src/pages/demos/app-bar/BottomAppBar.js

+          Inbox
+        </Typography>
+        <List className={classes.list}>
+          <ListSubheader className={classes.subHeader}>Today</ListSubheader>


It might be nice to have a second sub-header for "Yesterday". You could add them conditionally above the ListItem based on the id, for example 'Today' before. 1, and 'Yesterday' before 3.

mbrookes · 2018-10-08T18:00:43Z

docs/src/pages/demos/app-bar/BottomAppBar.js

+          <ListSubheader className={classes.subHeader}>Today</ListSubheader>
+          {messages.map(({ id, primary, secondary }) => (
+            <ListItem key={id} button>
+              <Avatar alt="Remy Sharp" src="/static/images/remy.jpg" />


Another nice-to-have, but perhaps we could have a couple more avatar images, and add the names to the messages object? Does the recipient only have one friend? 😆

-src="/static/images/remy.jpg" +src="{`/static/images/${user}.jpg`}"

There are only 2 images of people remy.jpg and uxceo-128.jpg in the images/ folder, Can I add more image assets in their, maybe the profile pictures of members of Material-UI?

adeelibr · 2018-10-09T17:08:51Z

This is how the updated demo looks at the moment.

I added my picture at the bottom, 😅 I hope it's okay. 😄

mbrookes · 2018-10-09T19:41:37Z

Haha.

Sure, why not one more. No need to add the whole team though, just assign those three to the messages at random.

Edit: just a thought though, do you have a color version for consistency with the others?

adeelibr · 2018-10-10T03:39:10Z

I have updated the demo, can you kindly review again 😅

mbrookes · 2018-10-10T09:29:23Z

This is going to seem very pedantic, but could you assign the avatars randomly 🎲 - it will look more natural.

adeelibr · 2018-10-10T18:45:41Z

Sir I have updated the demo, is this better now? @mbrookes

mbrookes · 2018-10-10T19:43:07Z

I was afraid that might happen if I didn't spell it out. My fault. 😅

Please revert to assigning them in the array, but randomly. i.e. not 1, 2, 3, 1, 2, 3...

I also notice a couple of other niggles:

The spec says (paraphrasing) that when content overflows, it should be partially obscured, so that it's obvious that there is more content. At the moment the first two messages fit perfectly. Perhaps change the height of the demo, or reorder the messages so that you get 2 and a bit messages showing.
The Paper needs to be square:

adeelibr · 2018-10-11T03:19:06Z

I have updated the demo;

random images order
paper with square prop
increased the container height to show a bit more as suggested.

I have also synced this with the latest master branch.

Stale

mbrookes · 2018-10-11T16:32:53Z

@adeelibr Nice job! Just compare the end result with the first draft. 👌

adeelibr · 2018-10-11T18:16:26Z

@mbrookes thank you for being so helpful, with the code reviews. Working on Material-UI has been a great learning experience and with every code review and suggestion that I get from the team members here. I just improve a lot. So thank you for giving me an opportunity to work on this.

mbrookes · 2018-10-11T22:22:07Z

@mui-org/core-contributors Any last comments?

eps1lon

Some visual distinction between ListSubHeader and ListItem would be nice. Looks bad when text is scrolling below it. I think some divider below is sufficient.

mbrookes · 2018-10-12T14:29:15Z

@eps1lon It's how the pinned sub-header behaves in the list demo. List sub-headers don't have a divider in the spec, although I couldn't see an example of pinned sub-headers. Lets park this for now, and keep the examples consistent.

Feel free to open an issue if you think we can improve the ListSubheader component.

mbrookes · 2018-10-12T14:30:15Z

🎉

adeelibr added 2 commits September 28, 2018 21:15

added demo for bottom appbar

1b6383b

ran yarn prettier

181ebe6

oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Sep 29, 2018

mbrookes changed the title ~~[Appbar] Added Demo For A Bottom Appbar~~ [Appbar] Add demo for a bottom App Bar Sep 29, 2018

mbrookes changed the title ~~[Appbar] Add demo for a bottom App Bar~~ [docs] Add demo for a bottom app bar Sep 29, 2018

mbrookes requested changes Oct 2, 2018

View reviewed changes

adeelibr added 3 commits October 4, 2018 23:15

updated demo title

81448c1

Merge branch 'master' into feature-appbar-bottom-demo

f5376c9

updated example

1d8214d

adeelibr added 2 commits October 7, 2018 11:48

Merge branch 'master' into feature-appbar-bottom-demo

f098f64

update bottom app bar content

243d9cb

ran prettier

5c6d448

removed title, made fab button centered

35b5ce2

changes

8879bd0

adeelibr added 2 commits October 8, 2018 22:32

added background for listsubheader, added some bottom margin for list

79faa8c

Merge branch 'master' into feature-appbar-bottom-demo

5e4c7cd

mbrookes previously requested changes Oct 8, 2018

View reviewed changes

updated demo

19eceb1

upadted demo'

0369de3

updated demo

7d38279

adeelibr added 3 commits October 11, 2018 08:14

updated demo

365128e

Merge branch 'master' into feature-appbar-bottom-demo

f0eb692

removed commented code

8e737e9

mbrookes removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Oct 11, 2018

mbrookes approved these changes Oct 11, 2018

View reviewed changes

mbrookes added the docs Improvements or additions to the documentation label Oct 11, 2018

eps1lon reviewed Oct 12, 2018

View reviewed changes

mbrookes merged commit 7683886 into mui:master Oct 12, 2018

adeelibr deleted the feature-appbar-bottom-demo branch October 12, 2018 15:03

[docs] Add demo for a bottom app bar #13030

[docs] Add demo for a bottom app bar #13030

Conversation

adeelibr commented Sep 28, 2018 • edited by eps1lon Loading

adeelibr commented Oct 2, 2018

oliviertassinari commented Oct 2, 2018

adeelibr commented Oct 2, 2018

oliviertassinari commented Oct 2, 2018

mbrookes commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adeelibr commented Oct 4, 2018

mbrookes commented Oct 4, 2018 • edited Loading

adeelibr commented Oct 7, 2018

adeelibr commented Oct 7, 2018

mbrookes commented Oct 7, 2018

adeelibr commented Oct 7, 2018

adeelibr commented Oct 7, 2018 • edited Loading

mbrookes commented Oct 7, 2018 • edited Loading

mbrookes commented Oct 7, 2018

adeelibr commented Oct 8, 2018

mbrookes commented Oct 8, 2018

adeelibr commented Oct 8, 2018

mbrookes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

adeelibr Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

adeelibr commented Oct 9, 2018

mbrookes commented Oct 9, 2018 • edited Loading

adeelibr commented Oct 10, 2018

mbrookes commented Oct 10, 2018

adeelibr commented Oct 10, 2018

mbrookes commented Oct 10, 2018

adeelibr commented Oct 11, 2018

mbrookes commented Oct 11, 2018

adeelibr commented Oct 11, 2018

mbrookes commented Oct 11, 2018

eps1lon left a comment

Choose a reason for hiding this comment

mbrookes commented Oct 12, 2018

mbrookes commented Oct 12, 2018

adeelibr commented Sep 28, 2018 •

edited by eps1lon

Loading

mbrookes commented Oct 4, 2018 •

edited

Loading

adeelibr commented Oct 7, 2018 •

edited

Loading

mbrookes commented Oct 7, 2018 •

edited

Loading

mbrookes Oct 8, 2018 •

edited

Loading

adeelibr Oct 9, 2018 •

edited

Loading

mbrookes commented Oct 9, 2018 •

edited

Loading