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

Add link theme for vaadin-tab #82

Closed
wants to merge 1 commit into from
Closed

Conversation

web-padawan
Copy link
Member

@web-padawan web-padawan commented Feb 9, 2018

Fixes #81

This feature was inspired by work on the elements-viewer navigation where I had to use absolute positioning to achieve what I needed: https://github.com/web-padawan/elements-viewer/blob/master/src/viewer-link.html#L13

@jouni PTAL


This change is Reviewable

Copy link
Member

@jouni jouni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of an explicit theme="link", I would just use negative margin on the slotted a element.

@web-padawan
Copy link
Member Author

Instead of an explicit theme="link", I would just use negative margin on the slotted a element.

Done.


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@heruan
Copy link
Member

heruan commented Feb 14, 2018

I think it's important then to check if the click event has special keys involved (e.g. control/meta) since in that case the tab selection should not occur since the link could be opened in a new browser tab/window.

@web-padawan
Copy link
Member Author

Closing this now as the theme-only change is obviously not enough. Will figure out a better way to implement this later.

@heruan
Copy link
Member

heruan commented Feb 14, 2018

Could this PR + vaadin/vaadin-list-mixin/pull/30 be a solution? To this I'd force the color of links and add the style for hovered ones, e.g.

      :host ::slotted(a) {
        display: flex;
        align-items: center;
        width: calc(100% + 2em);
        height: 100%;
        margin: -.25em -1em;
        padding: .25em 1em;
        text-decoration: none;
-       color: inherit;
+       color: inherit !important;
        outline: none;
      }
+     :host ::slotted(a:hover) {
+       text-decoration: none !important;
+     }

Also, icons inside the anchor should be styled the same as icons inside the tab (aside, on top, etc.).

@heruan
Copy link
Member

heruan commented Feb 14, 2018

Pretty much this: vaadin/vaadin-tabs/compare/master...heruan:links-in-tab 😄

@jouni
Copy link
Member

jouni commented Feb 15, 2018

::slotted(a) > * are unsupported selectors. You can only match the first hierarchy level of the slotted content, AFAIK.

@heruan
Copy link
Member

heruan commented Feb 15, 2018

Interesting, somehow it renders correctly: https://jsfiddle.net/heruan/rz60k8a2/

From the inspector it seems the selector matches, at least in Safari and Firefox. Out of the spec but supported by browsers, maybe?

@jouni
Copy link
Member

jouni commented Feb 16, 2018

Firefox is still polyfilled (no native shadow), so that’s why it works – the polyfill doesn’t strip the stuff after ::slotted(). Could be a a bug in Safari, as I do remember seeing inconsistent behavior regarding this between Chrome and Safari. So I would not at least expect it to work reliably.

@heruan
Copy link
Member

heruan commented Feb 16, 2018

Got it, thank you! Tested in Chrome too and it works, but I understand it's not reliable if out of the spec.

I've placed this on a local theme-for style and I'm happy with it for the moment (there's still the "click with modifiers" issue, see vaadin/vaadin-list-mixin/pull/30), at least until someone comes up with a more reliable solution!

@web-padawan web-padawan deleted the lumo-theme-link branch August 20, 2018 13:42
@web-padawan web-padawan restored the lumo-theme-link branch February 21, 2019 13:41
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

3 participants