search

material-components / material-components-web-catalog

Catalog of Material Components for the web (MDC Web)
https://material-components.github.io/material-components-web-catalog/
Apache License 2.0
116 stars 38 forks source link

New Hero/Code Snippet prototype #233

Closed williamernest closed 5 years ago

williamernest commented 5 years ago

Implements code snippets in the hero area for button, card, text field, and top app bar.

Also fixes #222.

williamernest commented 5 years ago

When react re-renders the button it removes the mdc-ripple--upgraded class, but even when there's logic to keep the class on re-render the enhanced ripple stops working the moment the ripple is changed. I'm not sure why it's not working, I'm not seeing any errors being logged.

Does the Ripple calculate it's size when the user clicks, or is there some kind of initialization going on ? I initially thought it was best to re-instantiate it since the size of the label may change resulting in a smaller/larger ripple.

kfranqueiro commented 5 years ago

When react re-renders the button it removes the mdc-ripple--upgraded class, but even when there's logic to keep the class on re-render the enhanced ripple stops working the moment the ripple is changed. I'm not sure why it's not working, I'm not seeing any errors being logged.

So like, the ripple just never appears? Do focus/hover still work? My only suspicion is that React might be completely replacing the DOM rather than just re-rendering text and attributes, and you're relying on MDCRipple's vanilla event handlers which were actually hooked up on an element that is no longer present.

Does the Ripple calculate it's size when the user clicks, or is there some kind of initialization going on ? I initially thought it was best to re-instantiate it since the size of the label may change resulting in a smaller/larger ripple.

Ripple calculates its size when the user clicks. Unbounded ripples additionally call layout the frame after init (and on window resize) so that hover/focus states are sized correctly, but this isn't an unbounded case.

williamernest commented 5 years ago

Focus/Hover still work. The event handlers are still the element. The branch now shows what happens if you want to pull it down. Maybe you have an idea since you know more about the ripple than I do.

googlebot commented 5 years ago

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot commented 5 years ago

CLAs look good, thanks!

moog16 commented 5 years ago

RE For some reason the "Properties" heading for Button and "Title" heading for Top App Bar is invisible in Chrome until I select the text. Firefox doesn't have this problem, and it doesn't happen for Text Field in Chrome. This is a weird one.

Cannot repro - can you help with more information?

williamernest commented 5 years ago

Re * At some point while using the catalog, the drawer and top app bar completely vanished on me. I'm not sure what caused it. I had to refresh to get it back.

This is reproducible on the current catalog. I just filed #247 to track it.

moog16 commented 5 years ago

RE Cards: maybe we can leave a comment in the code that there is some additional setup so that it is obvious. I think the more explicit the better.

RE hero-classes: I think we should be removing. What do you think @williamernest ?

RE top app bar snippets: if we remove the modifier classes like hero-, this should avoid this issue, correct?

williamernest commented 5 years ago

Re hero classes: I planned on taking a look at that today. We should definitely be removing them

moog16 commented 5 years ago

RE mdc-ripple-upgraded class: not seeing this happen. Which component specifically do you see this? I remember seeing this issue, but can't repro.

kfranqueiro commented 5 years ago

RE top app bar snippets: if we remove the modifier classes like hero-, this should avoid this issue, correct?

Not sure what you're referring to. My point is the mdc-top-app-bar--... modifier classes are being applied to the wrong element entirely because there's a wrapping div around the component.

RE mdc-ripple-upgraded class: not seeing this happen. Which component specifically do you see this? I remember seeing this issue, but can't repro.

Any component that has a ripple applied. Definitely button and card (primary-action).

moog16 commented 5 years ago

RE top app bar: gotcha - i didn't even notice that since it was working. The only top app bar that didn't work was always short.

RE mdc-ripple-upgraded: still not seeing that :| Maybe try running npm i? I can't imagine that would actually fix it.

Is there anything else outstanding? I think I got all the points besides the mdc-ripple-upgraded.

kfranqueiro commented 5 years ago

RE mdc-ripple-upgraded: still not seeing that :| Maybe try running npm i? I can't imagine that would actually fix it.

No it won't fix it, given that I had to run npm i to run this branch in the first place.

And yes, I can still reproduce it. I will outline it again, with a specific case:

  1. Go to the Button page
  2. View the Web tab
  3. Change the Variant
  4. The Web tab shows mdc-ripple-upgraded but shouldn't
  5. If you switch to another tab then back, the class will be pruned
williamernest commented 5 years ago

RE: Height of hero: It all fits above the fold on my 15" MBP with a few hundred pixels to spare (@100% zoom). We did need the extra space before we made leading/trailing icons chips. I can remove 114px from it and it looks better.