feat(top-app-bar): Switch to use variant specific foundations #2412
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2412 +/- ##
==========================================
- Coverage 98.89% 98.86% -0.03%
==========================================
Files 100 101 +1
Lines 4145 4151 +6
Branches 531 531
==========================================
+ Hits 4099 4104 +5
- Misses 46 47 +1
Continue to review full report at Codecov.
|
| ); | ||
|
|
||
| let foundation; | ||
| if (this.root_.classList.contains(cssClasses.SHORT_CLASS)) { |
There was a problem hiding this comment.
Have you run this by Lynn? Given our desire to avoid putting business logic on the component level, this seems to go against that, but I suppose having different components for each type would be overkill...
There was a problem hiding this comment.
We talked a bit about it over the past couple days. She asked me to create this PR so she could go over how I broke it up and start the discussion. I'm not sure where this block of code would really belong we don't want it in the Component.
There was a problem hiding this comment.
Actually I think this might be exactly the kind of logic we want to change per web platform
- Vanilla JavaScript: Read the DOM to decide which type of foundation to use
- React: Read a React prop to decide which type of foundation to use
- Internal Google JavaScript: Read some internal property available at initiate-time
| ); | ||
|
|
||
| let foundation; | ||
| if (this.root_.classList.contains(cssClasses.SHORT_CLASS)) { |
There was a problem hiding this comment.
Actually I think this might be exactly the kind of logic we want to change per web platform
- Vanilla JavaScript: Read the DOM to decide which type of foundation to use
- React: Read a React prop to decide which type of foundation to use
- Internal Google JavaScript: Read some internal property available at initiate-time
| } | ||
|
|
||
| init() { | ||
| const isAlwaysCollapsed = this.adapter_.hasClass(cssClasses.SHORT_COLLAPSED_CLASS); |
There was a problem hiding this comment.
I think you could call super.initi() and then not have to register the scroll handler
There was a problem hiding this comment.
This scroll handler is specific to this variant (short). The base foundation has the navigation icon click handler logic, since that will apply to all variants.
| } | ||
|
|
||
| destroy() { | ||
| this.adapter_.deregisterScrollHandler(this.scrollHandler_); |
No description provided.