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

[Migration] deprecate darkside props #3656

Merged
merged 6 commits into from
Mar 18, 2025

Conversation

JulianNymark
Copy link
Contributor

Description

Component Checklist 📝

  • JSDoc
  • Examples
  • Documentation / Decision Records
  • Storybook
  • Style mappings (@navikt/core/css/config/_mappings.js)
  • Component tokens (@navikt/core/css/tokens.json)
  • CSS class deprecations (@navikt/aksel-stylelint/src/deprecations.ts)
  • Exports (@navikt/core/react/src/index.ts and @navikt/core/react/package.json)
  • New component? CSS import (@navikt/core/css/index.css)
  • Breaking change? Update migration guide. Consider codemod.
  • Changeset (Format: <Component>: <gitmoji?> <Text>. E.g. "Button: ✨ Add feature xyz.")

Copy link

changeset-bot bot commented Mar 13, 2025

⚠️ No Changeset found

Latest commit: 2984195

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@JulianNymark JulianNymark changed the title 🚧 working impl 🚧 deprecate darkside props Mar 13, 2025
Copy link
Contributor

github-actions bot commented Mar 13, 2025

Storybook demo / Chromatic

7096cf5a2 | 91 components | 135 stories

@JulianNymark JulianNymark marked this pull request as ready for review March 17, 2025 09:14
@JulianNymark JulianNymark changed the title 🚧 deprecate darkside props [Migration] deprecate darkside props Mar 17, 2025
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might make sense to rename spacing key for the group to something more fitting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, can make a whole separate one for darkside? 🤔

Copy link
Collaborator

@KenAJoh KenAJoh left a comment

Choose a reason for hiding this comment

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

Seems the imports needs updating ❌

import type { API, FileInfo } from "jscodeshift";
import { getLineTerminator } from "../../../utils/lineterminator";
import removePropsFromComponent from "../../../utils/removeProps";
import { findComponentImport } from "../../spacing/spacing.utils";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KenAJoh should we move all the utility functions inside the spacing directory out to more generic utils?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you see the use for them elsewhere, sure 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only currently use one of the more generic ast crawling functions you made, but i moved the whole group of them into a separate ast.ts. Feels like that makes the most sense 💭

@JulianNymark
Copy link
Contributor Author

Would be nice to merge this and use the utils further in the current wip migration (Box) 🤩 Will merge ASAP as it's approved

@JulianNymark JulianNymark merged commit 9f958c1 into main Mar 18, 2025
6 checks passed
@JulianNymark JulianNymark deleted the darkside-migration-deprecate-props branch March 18, 2025 12:37
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