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

RFC: refactor Sass og innfør en stilguide #2868

Merged
merged 45 commits into from
Jun 7, 2022
Merged

RFC: refactor Sass og innfør en stilguide #2868

merged 45 commits into from
Jun 7, 2022

Conversation

wkillerud
Copy link
Contributor

@wkillerud wkillerud commented May 6, 2022

Hva skjer?

Ganske mye. I forbindelse med refactor til nytt modulsystem for Sass (#2757) ønsket jeg å se grundigere på den eksisterende Sass-kodebasen. Jeg synes vi har manglet et tydelig API, og at vi har vært inkonsekvente med et par ting.

Jeg noterte med ned disse endringsforslagene tidligere i år:

  • Gi alle mapper en _index.scss, vekk fra _all.scss (det er det vi ha indeksen til)
    • Inkludert rotnivå så folk kan @use "@fremtind/jkl-table" for eksempel
  • Være tydelige på hva som er privat og hva som er public
    • _-prefix i variabelnavn, mixins, og funksjoner
  • Namespace alle globale ting, som CSS-variabler og animasjoner, med jkl
  • Adopter SassDoc for public ting
  • Dokumenter alt dette som en Sass author style guide

Det er helt lov å være uenig i disse forslagene. Supert også om du har andre forslag til forbedringer nå som jeg uansett er i gang med en breaking change.

End goal

  • Alle Sass-filer i Jøkul får alt de trenger fra rotnivå av Core
  • Prosjektet har et tydelig Sass-API å forholde seg til, dokumentert i stilguiden

Hva skal jeg mene noe om?

Minimum

Bonus

The real MVP

  • Finner du noen bugs i noen av endringene mine? 🕵️‍♂️

TODO

  • Samle innspill på stilguiden og justere branchen
  • Sørge for at alle pakker som avhenger av core får en ny majorversjon ved publisering
  • Skrive migreringsguider

🎯 Sjekkliste

  • yarn build og yarn ci:test gir ingen feil

Import-syntaksen fjernes fra Sass i oktober. Vi ønsker å være litt
i forkant og migrere vekk fra syntaksen i god tid før det.

BREAKING CHANGE:
For brukere av Sass-koden vil dere kunne oppleve breaking changes. Se MIGRATION.md.
Om du importeres CSS i prosjektet ditt er det ingen breaking change.
BREAKING CHANGE:
Maps fra motion er gjort private, jkl-motion er nå easing, jkl-timing er nå timing.
BREAKING CHANGE:
Avhenger av ny majorversjon av core
For de som bruker Sass til å importere stilpakker, nå kan de slippe "jkl-button/button" for eksempel
Det fungerer dårlig med Sass sin string interpolation
BREAKING CHANGE:
Sass-variabler, mixins og CSS-animasjoner fra pakker annet enn core er gjort private
Er ikke kutyme i SassDoc virker det som
@wkillerud wkillerud self-assigned this May 6, 2022
@wkillerud wkillerud added 📚 Dokumentasjon Dokumentasjon i kodebasen, Figma eller portalen 👋 help wanted labels May 6, 2022
@fremtind-bot

This comment was marked as outdated.

fremtind-bot added a commit that referenced this pull request May 6, 2022
Liker ikke at Gulp "bygger" uten å klage på disse importene.
@fremtind-bot

This comment was marked as outdated.

fremtind-bot added a commit that referenced this pull request May 9, 2022
portal/src/texts/guider/sass-stilguide.mdx Show resolved Hide resolved
portal/src/texts/guider/sass-stilguide.mdx Outdated Show resolved Hide resolved
portal/src/texts/guider/sass-stilguide.mdx Outdated Show resolved Hide resolved
@fremtind-bot

This comment was marked as outdated.

fremtind-bot added a commit that referenced this pull request May 9, 2022
Det gjør det vanskeligere å søke etter navn man kjenner fra APIet utad.
Det gjør det også vanskeligere for enkelte verktøy å følge koden, for
eksempel Go to Definition i VS Code.
fremtind-bot added a commit that referenced this pull request May 16, 2022
@wkillerud
Copy link
Contributor Author

Benyttet sjansen til å innføre ESLint-regler for imports for å få en diff jeg kunne tagge med breaking change i React-koden. Promoterer samtidig de siste gjenstående pakkene vi har hatt på versjon 0.x.x til 1.0.0.

Changes:
 - @fremtind/jkl-action-changed-files: 1.0.1 => 1.0.2 (private)
 - @fremtind/jkl-accordion-react: 6.0.15 => 7.0.0
 - @fremtind/jkl-accordion: 6.0.13 => 7.0.0
 - @fremtind/jkl-alert-message-react: 7.0.17 => 8.0.0
 - @fremtind/jkl-alert-message: 5.0.23 => 6.0.0
 - @fremtind/jkl-breadcrumb-react: 1.0.12 => 2.0.0
 - @fremtind/jkl-breadcrumb: 1.0.12 => 2.0.0
 - @fremtind/browserslist-config-jkl: 1.0.0-alpha.0 => 1.0.0
 - @fremtind/jkl-button-react: 10.1.12 => 11.0.0
 - @fremtind/jkl-button: 7.0.13 => 8.0.0
 - @fremtind/jkl-card-react: 5.2.16 => 6.0.0
 - @fremtind/jkl-card: 5.1.11 => 6.0.0
 - @fremtind/jkl-checkbox-react: 6.1.13 => 7.0.0
 - @fremtind/jkl-checkbox: 6.0.13 => 7.0.0
 - @fremtind/jkl-constants-util: 1.0.0-alpha.0 => 1.0.0
 - @fremtind/jkl-content-toggle-react: 2.0.26 => 3.0.0
 - @fremtind/jkl-content-toggle: 2.0.23 => 3.0.0
 - @fremtind/jkl-cookie-consent-react: 5.1.35 => 6.0.0
 - @fremtind/jkl-cookie-consent: 6.0.13 => 7.0.0
 - @fremtind/jkl-core: 9.6.1 => 10.0.0
 - @fremtind/jkl-datepicker-react: 9.1.2 => 10.0.0
 - @fremtind/jkl-datepicker: 6.0.16 => 7.0.0
 - @fremtind/jkl-description-list-react: 4.0.26 => 5.0.0
 - @fremtind/jkl-description-list: 4.0.23 => 5.0.0
 - @fremtind/jkl-expand-button-react: 2.0.5 => 3.0.0
 - @fremtind/jkl-expand-button: 1.2.14 => 2.0.0
 - @fremtind/jkl-feedback-react: 12.1.2 => 13.0.0
 - @fremtind/jkl-feedback: 9.0.13 => 10.0.0
 - @fremtind/jkl-field-group-react: 5.1.1 => 6.0.0
 - @fremtind/jkl-field-group: 4.0.23 => 5.0.0
 - @fremtind/jkl-footer-react: 1.0.9 => 2.0.0
 - @fremtind/jkl-footer: 1.0.6 => 2.0.0
 - @fremtind/jkl-formatters-util: 2.0.10 => 2.0.11
 - @fremtind/jkl-hamburger-react: 8.0.15 => 9.0.0
 - @fremtind/jkl-hamburger: 9.0.13 => 10.0.0
 - @fremtind/jkl-icon-button-react: 1.0.0-alpha.0 => 1.0.0
 - @fremtind/jkl-icon-button: 1.0.0-alpha.0 => 1.0.0
 - @fremtind/jkl-icons-react: 4.1.19 => 5.0.0
 - @fremtind/jkl-icons: 4.0.23 => 5.0.0
 - @fremtind/jkl-image-react: 2.0.16 => 3.0.0
 - @fremtind/jkl-image: 2.0.13 => 3.0.0
 - @fremtind/jkl-list-react: 6.1.21 => 7.0.0
 - @fremtind/jkl-list: 6.0.23 => 7.0.0
 - @fremtind/jkl-loader-react: 7.2.0 => 8.0.0
 - @fremtind/jkl-loader: 7.2.0 => 8.0.0
 - @fremtind/jkl-logo-react: 9.0.1 => 10.0.0
 - @fremtind/jkl-logo: 7.1.1 => 8.0.0
 - @fremtind/jkl-message-box-react: 6.3.14 => 7.0.0
 - @fremtind/jkl-message-box: 5.2.10 => 6.0.0
 - @fremtind/jkl-radio-button-react: 6.2.1 => 7.0.0
 - @fremtind/jkl-radio-button: 6.0.13 => 7.0.0
 - @fremtind/jkl-react-hooks: 8.2.3 => 9.0.0
 - @fremtind/jkl-select-react: 9.2.1 => 10.0.0
 - @fremtind/jkl-select: 7.0.13 => 8.0.0
 - @fremtind/stylelint-config-jkl: 2.0.6 => 2.0.7
 - @fremtind/jkl-summary-table-react: 6.0.18 => 7.0.0
 - @fremtind/jkl-summary-table: 6.0.16 => 7.0.0
 - @fremtind/jkl-table-react: 7.5.7 => 8.0.0
 - @fremtind/jkl-table: 5.5.6 => 6.0.0
 - @fremtind/jkl-tabs-react: 1.3.23 => 2.0.0
 - @fremtind/jkl-tabs: 1.2.20 => 2.0.0
 - @fremtind/jkl-tag-react: 1.1.26 => 2.0.0
 - @fremtind/jkl-tag: 1.1.23 => 2.0.0
 - @fremtind/jkl-text-input-react: 9.1.2 => 10.0.0
 - @fremtind/jkl-text-input: 7.0.16 => 8.0.0
 - @fremtind/jkl-toggle-switch-react: 8.0.1 => 9.0.0
 - @fremtind/jkl-toggle-switch: 8.0.1 => 9.0.0
 - @fremtind/jkl-validators-util: 2.3.3 => 2.3.4
 - @fremtind/jkl-webfonts: 1.1.10 => 2.0.0
 - @fremtind/portal: 12.5.0 => 13.0.0 (private)

BREAKING CHANGE:
Pakken avhenger av en breaking versjon av core
BREAKING CHANGE:
Ingen faktisk breaking change.
fremtind-bot added a commit that referenced this pull request May 18, 2022
fremtind-bot added a commit that referenced this pull request May 18, 2022
kennidenni
kennidenni previously approved these changes May 18, 2022
Det brukes bare i devserver/eksempler, ikke i selve bundlen som shippes
fremtind-bot added a commit that referenced this pull request Jun 7, 2022
@wkillerud wkillerud merged commit 5bd8b24 into main Jun 7, 2022
@wkillerud wkillerud deleted the refactor/sass branch June 7, 2022 07:23
github-actions bot pushed a commit that referenced this pull request Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 Dokumentasjon Dokumentasjon i kodebasen, Figma eller portalen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skriv om all bruk av Sass @import til Sass modules
3 participants