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

fix(tabset): hovering a tab doesn't affect the other tabs (#540) #651

Merged
merged 4 commits into from
Aug 24, 2018
Merged

fix(tabset): hovering a tab doesn't affect the other tabs (#540) #651

merged 4 commits into from
Aug 24, 2018

Conversation

cmajsmith
Copy link
Contributor

Please read and mark the following check list before creating a pull request:

Short description of what this resolves:

This is a fix for #540 issue.

@nnixaa
Copy link
Collaborator

nnixaa commented Aug 20, 2018

Hey @cmajsmith, thanks for the pr! I'm trying to match the description with the PR code, could you please elaborate how this is working? Thanks.

@cmajsmith
Copy link
Contributor Author

Sure.
So, the problem is following. Tabset component has a hovered tab text switching to bold which leads to the change of width of the container and so the sibling tabs being pushed and move. Please check the screenshot below:
nebular-tabset-hover

The fix provided adds a hidden pseudo-element ::before that contains bolded text, which makes the width of the tab constant. Technically it is done the following way:

  1. I've added the data-title attribute to the span in the tab which contains tab text;
  2. then (in CSS) the value of that attribute is taken and inserted as a content() to ::before pseudo-element;
  3. that ::before has font-weight set to bold (nb-theme(tabs-active-font-weight);) and is hidden with visibility: hidden;

data-title was taken instead of common title to avoid possibly unwanted tooltip showing/appearing.

Hope it helps.

@nnixaa
Copy link
Collaborator

nnixaa commented Aug 21, 2018

@cmajsmith ah, now I see it, thanks for the explanation! Is this a cross-browser way of fixing such issues?

@cmajsmith
Copy link
Contributor Author

@nnixaa, you're warmly welcome. I didn't test in all the browsers, but Can I use says: Yes!

@nnixaa
Copy link
Collaborator

nnixaa commented Aug 22, 2018

Hey @cmajsmith, got it. Though I personally feel like this is a bit fragile solution, and can be easily broken. Moreover, we have to introduce a new element in the dom, which is not particularly hidden.
What if we replace the bold font with a text-shadow instead? Like the second solution here https://www.sitepoint.com/quick-tip-fixing-font-weight-problem-hover-states/. We can even use the currentColor css value to not repeat the color in the shadow declaration. What do you think?

@cmajsmith
Copy link
Contributor Author

Hi, @nnixaa!

I've considered using the solution you've provided before settling with the one I've applied. The problem with text-shadow is that it inevitably adds blur to the text and it looks rather ugly.

To be honest, I think both (that text-shadow one and one in this PR) solutions are hacky. And thinking about the nature of the issue and ways to solve it I more and more get convinced that the best solution is... to get rid of bold styling for :hover state.

Assuming the inactive and unhovered tab is pale and becomes darker on hover - the control response will be enough to be noticed. The shift of neighboring tabs on click (caused with active tab turning into bold) will still occur, but it won't be as noticeable as on :hover. The question here is: does it align with your team's design guidelines? If it is - that would be cleanest fix ever. What do you think?

@nnixaa
Copy link
Collaborator

nnixaa commented Aug 22, 2018

@cmajsmith I completely agree with you, let me check with the design team.

@nnixaa
Copy link
Collaborator

nnixaa commented Aug 22, 2018

Hey @cmajsmith, we discussed this and agreed that we can remove bold font-weight from the :hover state on tabs. However, we need to do this in both tabs components (tabset and route-tabset). Would you like to complete this PR considering this?

@cmajsmith
Copy link
Contributor Author

@nnixaa sure! I'll do it tomorrow once will have some time on my hands. Will update this PR once done.

@cmajsmith
Copy link
Contributor Author

@nnixaa, seems like that is it.

@nnixaa
Copy link
Collaborator

nnixaa commented Aug 24, 2018

@cmajsmith great! Let's also remove the route-tabs-active-font-weight variable from _default.scss, _cosmic.scss and _corporate.scss if it is not used anywhere else.

@cmajsmith
Copy link
Contributor Author

@nnixaa, it is still used for making active tab (both in tabset and route-tabset) bold. Bold was removed for hover only.

@nnixaa
Copy link
Collaborator

nnixaa commented Aug 24, 2018

@cmajsmith exactly! Merging then.

@nnixaa nnixaa merged commit 22e39eb into akveo:master Aug 24, 2018
@cmajsmith
Copy link
Contributor Author

Hurray! Thanks @nnixaa for your time and help :)

@cmajsmith cmajsmith deleted the fix/tabset-hovers branch August 24, 2018 11:55
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.

2 participants