search

weaveworks / weave-gitops

Weave GitOps provides insights into your application deployments, and makes continuous delivery with GitOps easier to adopt and scale across your teams.
https://docs.gitops.weave.works/
Apache License 2.0
906 stars 152 forks source link

Letter spacing in Tabs text #3884

Closed mmoulian closed 11 months ago

mmoulian commented 1 year ago

Text inside the tabs have to follow the same "Letter Spacing Style" as the others components

Tabs have 1 pixel of letter spacing each (12/ semibold/ 1px)

Image of the Demo: Captura de Pantalla 2023-07-25 a las 17.52.19.png

Image of the Figma files: Captura de Pantalla 2023-07-25 a las 17.57.34.png

See Design System: https://www.figma.com/file/gnodK9xtoOahXBqa6ONsA5/WW-Design-System?type=design&node-id=2907%3A8713&mode=design&t=gJz0fdnU97xYQisY-1

jpellizzari commented 1 year ago

@mmoulian What is the deliverable here? Are the tab components "out of compliance" with the Figma files, and if so, how can we fix it.

mmoulian commented 1 year ago

@jpellizzari This is a small detail. I will try to explain myself better. All components (buttons and sidebar) have 1px letter spacing in the text, but tabs don't (at least I can't see it visually or in code)

Captura de Pantalla 2023-08-16 a las 14 47 26 Captura de Pantalla 2023-08-16 a las 14 53 31
opudrovs commented 1 year ago

@jpellizzari @mmoulian if this issue is OK to pick up, I can add the required letter spacing. I need a couple of small issues to work on until Monday or Tuesday.

jpellizzari commented 1 year ago

@opudrovs Yes go ahead :)

opudrovs commented 1 year ago

@mmoulian I've checked for this issue in OSS and see that both locally built OSS and the staging demo os OSS has proper letter spacing of 1px in the tabs text. So, it must be an enterprise issue only.

Screenshot 2023-09-05 at 15 32 14

Will check it locally with the main branch of enterprise now.

opudrovs commented 1 year ago

As discussed with @joshri , the current issue might be fixed by the following PR: https://github.com/weaveworks/weave-gitops/pull/3989 which closes issue #3918

This PR moves styles for the SubRouterTabs component used by WGE from the ui/lib/theme.ts file in OSS to SubRouterTabs. So, this should add the required letter-spacing: 1px to tabs in WGE.

But to test if the current issue is fixed I need that PR merged first, because trying to update WGE from this PR branch throws an error for me, maybe because of the step "publish js library" skipped in the PR (and from the local files too).

mmoulian commented 11 months ago

Hi @opudrovs! I'm looking at the EE demo and it's fixed. Thank you!

opudrovs commented 11 months ago

@mmoulian great, thanks for verifying it!

(Actually, it was not much to do here, I just waited until the OSS PR which moved the tab styles to the component instead of global styles is merged and updated enterprise with the latest OSS + fixed a broken snapshot test.)