Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Material theme tabs #22

Closed
wants to merge 7 commits into from
Closed

Material theme tabs #22

wants to merge 7 commits into from

Conversation

h00k00
Copy link
Contributor

@h00k00 h00k00 commented Jan 3, 2018

Note: Using valo-icons still instead of material icons. Icons will be updated later


This change is Reviewable

@platosha
Copy link
Contributor

platosha commented Jan 3, 2018

Reviewed 5 of 6 files at r1.
Review status: 5 of 6 files reviewed at latest revision, 7 unresolved discussions.


a discussion (no related file):
Focus style in Tabs of Material Components for the Web highlights the tab background for hover and focus styles, and this feels very nice
✂-1

Right now, I can hardly see when a tab is focused. The same issue exists with <paper-tabs>. <paper-tabs> do not have background highlight on hover and focus.

The Guideline does not specify explicitly how tab focus and hover look too, unfortunately.

Not a part of the scope maybe, so this is not a blocker issue. Just an enhancement idea. IMO, would be nice to have hover and focus highlight the way Material Components Tabs have them.


a discussion (no related file):
Ripple on the underline does not feel right. Instead, the tab background should have a ripple visual click feedback, not the underline.

The Guidelines examples, <paper-tabs>, Tabs from Material Components, all of them visually highlight the tab background in response to the tab click, the underline just changes its position.


a discussion (no related file):
Expected: wrapped tab text is centered in the tab
✂-1

Actual: wrapped tab text is aligned to the left
✂-2


a discussion (no related file):
Expected: non-scrollable tabs are either stretched to viewport or aligned to the center:

✂-1

✂-2

Actual: <vaadin-tabs> are always left aligned:
✂-3

In fact, the Guideline specifically defines two distinct types of tabs: fixed tabs and scrollable tabs. We could have one type as default behavior, and another as a theme variation.

Since it’s not expected to adjust theme to enable scrolling, I would suggest that “scrollable” is a default look, and there is also an opt-in theme="fixed" variation improving the look when scrolling is unnecessary.


a discussion (no related file):
Expected: when scrolling buttons are not in use, the first tab starts exactly where the tab container starts, and there is no gap between. If the tab container has a background, the first tab touches the left side of the container:

✂-1

✂-2

Actual: there is a gap between the tabs container edge and the first tab edge:
✂-3


vaadin-tab.html, line 16 at r1 (raw file):
Does not match how The Guideline specifies min and max width:

Width minimum and maximum (inclusive of padding)
Maximum: 264dp
Minimum: 160dp for larger views, 72 dp for smaller views


vaadin-tabs.html, line 135 at r1 (raw file):

      }

      /* Pill theme */

Missing demo for the pill theme variation? I cannot find it.


Comments from Reviewable

@platosha
Copy link
Contributor

platosha commented Jan 3, 2018

Reviewed 1 of 6 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions.


Comments from Reviewable

@web-padawan
Copy link
Member

Review status: 3 of 6 files reviewed at latest revision, 8 unresolved discussions.


vaadin-tabs.html, line 91 at r2 (raw file):

        --_material-tabs-overflow-mask-image: none;
        -webkit-mask-image: var(--_material-tabs-overflow-mask-image);
        mask-image: var(--_material-tabs-overflow-mask-image);

Need to wrap this property (currently supported without prefix by Firefox only) into the condition to prevent Edge crash. Example: https://github.com/vaadin/vaadin-tabs/blob/master/theme/lumo/vaadin-tabs.html#L81


Comments from Reviewable

@tomivirkki
Copy link
Member

Merged in 02c8321

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants