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

Enable tooltips in NavLink and add minor CSS fixes for Navigation #186

Merged
merged 15 commits into from
Dec 7, 2023

Conversation

nadijagraca
Copy link
Contributor

Description

Minor css fixes on navlink and accordion components:

  • added lined between each accordion item (the line is not new design, but rather old that got lost in the refactoring)
  • adjusted highlighting of active icon
  • implemented and styled tooltip component

Screenshot

Screenshot 2023-11-29 at 19 32 55

Checklist

  • I have not referenced individuals, products or companies in any commits, directly or indirectly
  • I have not added data or restricted code in any commits, directly or indirectly
  • I have updated the docstring of any public function/class/model changed
  • I have added tests to cover my changes (if applicable)

Types of changes

  • Docs/refactoring (non-breaking change which improves codebase)
  • Bug fix (non-breaking change which fixes an issue)
  • [] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

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

I'm not fully familiar with this feature, but still was curious in it. So, I left few comments 😄.

vizro-core/examples/default/app.py Outdated Show resolved Hide resolved
vizro-core/examples/default/app.py Show resolved Hide resolved
vizro-core/src/vizro/models/_navigation/nav_link.py Outdated Show resolved Hide resolved
@antonymilne
Copy link
Contributor

antonymilne commented Nov 30, 2023

How is this different from #183? 🤔

I see it's the same, just a new PR. Let me copy my comments over to make sure they don't get lost:

  • update tests to pass. Let me know if you have any problems understanding the new assert_component_equals functionality that I introduced in my last PR, because I didn't get a chance to explain it to the team yet
  • update screenshots in docs - note these all use the dark theme now

@nadijagraca nadijagraca mentioned this pull request Nov 30, 2023
9 tasks
Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

Great stuff, thank you for all the fixes! 🙏

The tests look good. I just made a small change to make them a bit less verbose: 6500d8f. It's the same tests as you wrote, just putting some stuff into a variable to make it clearer.

Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

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

Hey @nadijagraca , great that you found a way to style the tooltips now! 🚀 Have some change requests mostly related to design and some default options.

It would be great if you could apply the Figma designs we have for tooltips here. I noticed some discrepancies. Ignore the comment, if the Figma designs are outdated and these are indeed our new designs?

According to the Figma designs, we would also need a fixed width for the tooltip, such that text wrapping is activated if the label is very long. I think that makes sense and should also be our default.

Could you double-check with @antonymilne whether the label indeed should be mandatory? For me it makes sense, that users can turn off the tooltips if they want. Currently that's not possible.

Could you update the accordion_inside_nav_bar.png such that it also shows the last page in the Accordion? In the new screenshot it's currently cut off, but I think it makes sense to keep it in, as we're explaining the grouping in the docs section

@huong-li-nguyen huong-li-nguyen changed the title adding tooltip functionality, small css fixes on navigation panel Enable tooltips in NavLink and add minor CSS fixes for Navigation Dec 5, 2023
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

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

The new tooltips look great now, love the box-shadowing on the light theme as well! 🚀 Thanks for incorporating all the changes :) Left one small comment and could you replace the screenshot as mentioned here:

Could you update the accordion_inside_nav_bar.png such that it also shows the last page in the Accordion? In the new screenshot it's currently cut off, but I think it makes sense to keep it in, as we're explaining the grouping in the docs section

The rest looks great! After updating the screenshot and just checking whether we need all the CSS properties defined in .mantine-Tooltip-tooltip , feel free to merge after all the tests pass.

vizro-core/src/vizro/static/css/layout.css Outdated Show resolved Hide resolved
@nadijagraca nadijagraca merged commit c1e9071 into main Dec 7, 2023
29 checks passed
@nadijagraca nadijagraca deleted the fix/navigation_css_refinements branch December 7, 2023 11:01
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

4 participants