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.

Sign up for GitHub

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

Jump to bottom

feat(@vtmn/css, @vtmn/svelte, @vtmn/react, @vtmn/vue): rework VtmnNavbarLink for SSR #1425

Merged
merged 5 commits into from
Jun 2, 2023
Merged

feat(@vtmn/css, @vtmn/svelte, @vtmn/react, @vtmn/vue): rework VtmnNavbarLink for SSR #1425

merged 5 commits into from
Jun 2, 2023

Conversation

Tlahey
Copy link
Contributor

@Tlahey Tlahey commented Apr 27, 2023 •
edited
Loading

BREAKING CHANGE: 'VtmnNavbarLink' remove properties label and showLabel, replaced by a span slot

Changes description

Move the Label property into slot in order to apply a sr-only with breakpoints.

Context

With SSR, it is not possible to detect the width of the device. So, in order to avoid flikering effects, we have to set the label on a slot in order to apply style show/hidden on it.

How it looks

image
image
image
image

On the navbar
image
image

Checklist

  • Make sure you are requesting to pull a topic/feature/bugfix branch. Please, don't request directly from your main!
  • Check commits & PR names matches our requested structure. It must follow the https://www.conventionalcommits.org pattern.
  • Check your code additions will fail neither code linting checks.
  • I have reviewed the submitted code.
  • I have tested on related showcases.
  • If it includes design changes, please ask for a review design-system-core-team-design GitHub team.

Does this introduce a breaking change?

  • Yes

Other information

Rework the css part and enhancer the CSS in order to match the figma with the VtmnBadge.

Current :
image

Target :
image
image

@Tlahey Tlahey added enhancement 🚀 New feature or request React 🔵 Related to React components library CSS 🎨 Related to CSS styles packages Svelte 🟠 Related to Svelte components library Vue 🟢 Related to Vue components library community 👥 As we stopped improvements for this version of Vitamin, this issue needs to be done by the community labels Apr 27, 2023
@Tlahey Tlahey self-assigned this Apr 27, 2023
@Tlahey Tlahey requested review from thibault-mahe and a team as code owners April 27, 2023 13:33
@Tlahey Tlahey mentioned this pull request Apr 27, 2023
Closed
@lauthieb lauthieb linked an issue Apr 27, 2023 that may be closed by this pull request
bug(@vtmn/svelte): VtmnNavbarLink is not SSR compliant #1424
Closed
@lauthieb lauthieb changed the title feat(@vtmn/css, @vtmn/svelte, @vtmn/react, @vtmn/vue): rework VtmnNavbarLink for SSR SSR feat(@vtmn/css, @vtmn/svelte, @vtmn/react, @vtmn/vue): rework VtmnNavbarLink for SSR Apr 27, 2023
lauthieb

This comment was marked as duplicate.

Sign in to view

lauthieb
lauthieb requested changes Apr 27, 2023
View reviewed changes
Copy link
Member

@lauthieb lauthieb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @Tlahey, for me that's ok but still a difference for the badge position:

CleanShot 2023-04-27 at 15 50 56@2x

Repro: yarn start:css & go to http://localhost:6006/?path=/story/components-navigation-navbar--overview

@Tlahey Tlahey requested a review from lauthieb April 27, 2023 16:08
lauthieb
lauthieb previously approved these changes Apr 27, 2023
View reviewed changes
Copy link
Member

@lauthieb lauthieb left a comment

Choose a reason for hiding this comment

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

That's better! Thanks @Tlahey

@Tlahey Tlahey dismissed stale reviews from thibault-mahe and lauthieb via 48547d0 May 11, 2023 06:51
@Tlahey Tlahey force-pushed the feat/vtmnNavbarLink-enhancement-ssr branch from 48547d0 to d679169 Compare May 23, 2023 07:30
@Tlahey Tlahey mentioned this pull request May 23, 2023
Merged
6 tasks
lauthieb
lauthieb previously approved these changes May 25, 2023
View reviewed changes
@lauthieb
Copy link
Member

lauthieb commented May 25, 2023 •
edited
Loading

OK for me! Thanks @Tlahey :)

We need to wait for #1431.

Please, be careful by adding correctly "BREAKING CHANGE:" in the body of the merge commit in order to ensure that we will bump to a major release thanks to Lerna automation.

Example with this merge commit: 25a7c16

@Tlahey Tlahey force-pushed the feat/vtmnNavbarLink-enhancement-ssr branch from 4439223 to f880bcc Compare June 2, 2023 10:12
@thibault-mahe thibault-mahe merged commit dc6b999 into main Jun 2, 2023
@thibault-mahe thibault-mahe deleted the feat/vtmnNavbarLink-enhancement-ssr branch June 2, 2023 12:26
@C0ZEN
Copy link
Contributor

C0ZEN commented Jun 5, 2023

Since it's a breaking change, how come it was released as 2.3.0 instead of 3.0.0?

@Tlahey
Copy link
Contributor Author

Tlahey commented Jun 5, 2023 •
edited
Loading

Totally agree, with the BREAKING CHANGE label, should have updated as a major version 🤔
Ping @thibault-mahe

thibault-mahe added a commit that referenced this pull request Jun 5, 2023
@thibault-mahe
Revert "feat(@vtmn/css, @vtmn/svelte, @vtmn/react, @vtmn/vue)…
f7c3f07 
…: rework `VtmnNavbarLink` for SSR (#1425)"

This reverts commit dc6b999.
@thibault-mahe thibault-mahe mentioned this pull request Jun 5, 2023
Merged
6 tasks
@thibault-mahe
Copy link
Contributor

I agree with you, something went wrong. I'm reverting the commits until I find some times to check on this: #1435

Thanks @C0ZEN :)

@Tlahey Tlahey restored the feat/vtmnNavbarLink-enhancement-ssr branch June 5, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thibault-mahe thibault-mahe thibault-mahe approved these changes

@lauthieb lauthieb lauthieb left review comments

Assignees

@Tlahey Tlahey

Labels
community 👥 As we stopped improvements for this version of Vitamin, this issue needs to be done by the community CSS 🎨 Related to CSS styles packages enhancement 🚀 New feature or request React 🔵 Related to React components library Svelte 🟠 Related to Svelte components library Vue 🟢 Related to Vue components library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug(@vtmn/svelte): VtmnNavbarLink is not SSR compliant
5 participants
@Tlahey @lauthieb @C0ZEN @thibault-mahe @Mig-l