feat(tab-scroller): Add scroll content width method for use in tab bar #3222
Conversation
|
All 128 screenshot tests passed for commit 9cbad57 vs. |
Codecov Report
@@ Coverage Diff @@
## feat/tabs/tabs #3222 +/- ##
==================================================
+ Coverage 98.28% 98.28% +<.01%
==================================================
Files 114 114
Lines 4831 4835 +4
Branches 603 604 +1
==================================================
+ Hits 4748 4752 +4
Misses 83 83
Continue to review full report at Codecov.
|
|
All 128 screenshot tests passed for commit 5205f66 vs. |
packages/mdc-tab-scroller/README.md
Outdated
| @@ -111,3 +115,4 @@ Method Signature | Description | |||
| `scrollTo(scrollX: number) => void` | Scrolls to the `scrollX` value. | |||
| `incrementScroll(scrollX: number) => void` | Increments the current scroll value by the `scrollX` value. | |||
| `getScrollPosition() => number` | Returns the current visual scroll position. | |||
| `getScrollContentWidth() => number` | Returns the width of the scroll content element. | |||
There was a problem hiding this comment.
Is this actually needed on the foundation, or could we simply write a one-liner referencing the correct node in the component?
|
Could definitely write a one-liner referencing the appropriate node.
…On Thu, Jul 26, 2018 at 7:50 AM Kenneth G. Franqueiro < ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In packages/mdc-tab-scroller/README.md
<#3222 (comment)>
:
> @@ -111,3 +115,4 @@ Method Signature | Description
`scrollTo(scrollX: number) => void` | Scrolls to the `scrollX` value.
`incrementScroll(scrollX: number) => void` | Increments the current scroll value by the `scrollX` value.
`getScrollPosition() => number` | Returns the current visual scroll position.
+`getScrollContentWidth() => number` | Returns the width of the scroll content element.
Is this actually needed on the foundation, or could we simply write a
one-liner referencing the correct node in the component?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3222 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFqhdqCGZfIDAnQRLhlj0udPhXvmPUXAks5uKdc5gaJpZM4Vg8BU>
.
|
packages/mdc-tab-scroller/README.md
Outdated
| @@ -65,6 +65,9 @@ CSS Class | Description | |||
| `mdc-tab-scroller` | Mandatory. Contains the tab scroller content. | |||
| `mdc-tab-scroller__scroll-area` | Mandatory. Denotes the scrolling area. | |||
| `mdc-tab-scroller__scroll-content` | Mandatory. Denotes the scrolling content. | |||
| `mdc-tab-scroller--start-aligned` | Optional. Sets the elements inside the scroll content element to be aligned to the start of the scroll content element. | |||
There was a problem hiding this comment.
Nit: --align-start, --align-end, and --align-center would...align...with modifier classes we have on a few other components
| * Returns the width of the scroll content | ||
| * @return {number} | ||
| */ | ||
| getScrollContentWidth() { |
There was a problem hiding this comment.
I agree with Ken on this one. I don't think this method is needed, and can just be a one liner in the component.
There was a problem hiding this comment.
I don't disagree. Since this is a subcomponent that's required inside of another, we have yet to figure out a pattern for accessing the child elements of a subcomponent.
| @@ -59,6 +59,18 @@ | |||
| will-change: transform; | |||
| } | |||
|
|
|||
| .mdc-tab-scroller--start-aligned .mdc-tab-scroller__scroll-content { | |||
There was a problem hiding this comment.
Instead of having all three (start, end, center), could you just have 2 where 1 is the default?
There was a problem hiding this comment.
The default is that they fill the horizontal space completely with no specific alignment. These three options are all optional.
|
All 128 screenshot tests passed for commit 7f2a698 vs. |
| @@ -59,6 +59,18 @@ | |||
| will-change: transform; | |||
| } | |||
|
|
|||
| .mdc-tab-scroller--start-aligned .mdc-tab-scroller__scroll-content { | |||
There was a problem hiding this comment.
The docs were updated but these classes were not
There was a problem hiding this comment.
lololol i'm a fool 😮
|
All 128 screenshot tests passed for commit f3c0c55 vs. |
|
All 128 screenshot tests passed for commit dda2915 vs. |
WIP fixed bg coloring of icons fix(tab-indicator): Use absolute positioning (#2547) WIP start of tab scroller WIP fixed transition duration WIP progress on scroller WIP added demos back chore(tabs): Removed tab scroller feat(tabs): Add tab indicator inside tab (#2565) feat(tab-scroller): Add tab scroller (#2577) Merge master into feat/tabs/tabs (#3096) feat(tab): Update tab color and typography (#3108) docs(tabs): Update metadata and synopses (#3117) feat(tab): Add MDCTabDimensions computation (#3122) feat(tab): Emit selection and activation events (#3139) docs(tabs): Update new READMEs to match standard (#3142) feat(tab): Give focus to tab when activated (#3164) feat(tab): Add mixin for parent positioning; Use mixin in tab scroller (#3179) fix(tabs): Suppress area occupied by scrollbar on platforms that show it (#3149) fix(tab): Remove extraneous padding from the stacked text label in LTR (#3193) feat(tabs): Add missing docs and create helper util API (#3194) Merge master into feat/tabs/tabs (#3227) feat(tab): Update layout; Add fixed-width mixin; Add min-width class (#3220) fix(tab-scroller): Fix incorrect animation stopping scroll value in RTL (#3237) feat(tab-scroller): Add scroll content width method for use in tab bar (#3222) feat(tab): Remove activation event emitting (#3242) feat(tab-bar): Add tab bar (#3229)
Expose method on foundation and component to get the width of the scroll content element. Add tests for newly exposed scroll content width methods. Add classes for adjusting layout of scroll content child elements. Update test coverage.