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

LOOM-1281 BpkDialog className moved to new wrapper span #3325

Merged
merged 3 commits into from
Apr 2, 2024
Merged

Conversation

Sybsw
Copy link
Contributor

@Sybsw Sybsw commented Mar 28, 2024

Part of work moving className usage out of backpack components.
Moved className from BpkCloseButton out to wrapper span, adjusted margins slightly to compensate shift

Remember to include the following changes:

  • Ensure the PR title includes the name of the component you are changing so it's clear in the release notes for consumers of the changes in the version e.g [KOA-123][BpkButton] Updating the colour
  • README.md (If you have created a new component)
  • Component README.md
  • Tests
  • Storybook examples created/updated
  • Type declaration (.d.ts) files updated
  • For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here

@Sybsw Sybsw added the patch Patch production bug label Mar 28, 2024
console.warn('BpkDialog: dismissible is true but no onClose prop was provided. Dialog will not be dismissible.');
console.warn(
'BpkDialog: dismissible is true but no onClose prop was provided. Dialog will not be dismissible.',
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prettier changed these lines here

@@ -32,11 +32,13 @@

&__close-button {
float: right;
margin: 0 0 tokens.bpk-spacing-base() tokens.bpk-spacing-base();
margin: 0 calc(tokens.bpk-spacing-sm() / 2) tokens.bpk-spacing-base()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linter required using calc or math.div for division

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, in another PR this wasn't needed and could be done via a variable: https://github.com/Skyscanner/backpack/pull/3320/files#r1542792398

Maybe we want to work on aligning and having a consistent approach throughout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed in slack thread, discovered that the other PR's change does have the same linter warnings so changing this to a variable will not do anything to solve that problem

Copy link

github-actions bot commented Mar 28, 2024

Warnings
⚠️

Package source files (e.g. packages/package-name/src/Component.tsx) were updated, but type files weren't. Have you checked that no types have changed?

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by 🚫 dangerJS against 14e0f12

Copy link

Visit https://backpack.github.io/storybook-prs/3325 to see this build running in a browser.

@Sybsw
Copy link
Contributor Author

Sybsw commented Mar 28, 2024

I have verified this change manually locally on storybook compared to a previous build, but I've just realised that there are no Percy tests for bpk-component-dialog

Edit: No visual tests is OK, it is because this component requires a click to see

@Sybsw Sybsw changed the title BpkDialog className moved to new wrapper span LOOM-1281 BpkDialog className moved to new wrapper span Mar 28, 2024
Copy link

github-actions bot commented Apr 2, 2024

Visit https://backpack.github.io/storybook-prs/3325 to see this build running in a browser.

@Sybsw Sybsw merged commit dd9b2d2 into main Apr 2, 2024
9 checks passed
@Sybsw Sybsw deleted the LOOM-1281 branch April 2, 2024 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Patch production bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants