-
Notifications
You must be signed in to change notification settings - Fork 183
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-1302] - Update BpkNavigationBarButtonLink and Icon #3361
Conversation
Visit https://backpack.github.io/storybook-prs/3361 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3361 to see this build running in a browser. |
|
||
import { BAR_STYLES, type BarStyle } from './BpkNavigationBar'; | ||
|
||
import STYLES from './BpkNavigationBarButtonLink.module.scss'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can rely on the Style for BpkButtonLink now. We don't need to replicate it here.
@@ -19,7 +19,7 @@ | |||
import type { ComponentType, MouseEvent, ReactNode } from 'react'; | |||
|
|||
// @ts-expect-error Untyped import. See `decisions/imports-ts-suppressions.md`. | |||
import BpkIconButton from '../../bpk-component-close-button'; | |||
import BpkCloseButton from '../../bpk-component-close-button'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong name❓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the expect-error
still valid with this changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the close-button has not been ported to typescript yet. So still needed.
</BpkButtonLink> | ||
<span className={className}> | ||
<BpkButtonLink | ||
alternate={barStyle === BAR_STYLES.onDark} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than setting our own class names we can use the BpkButtonLink's alterernate prop that flips the style.
customIcon={icon} | ||
// TODO: className to be removed | ||
// eslint-disable-next-line @skyscanner/rules/forbid-component-props | ||
className={getClassName( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear, we're leaving this className and the comment for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the BpkCloseButton is out of scope for this ticket. Otherwise would have to update everything else that uses it.
Visit https://backpack.github.io/storybook-prs/3361 to see this build running in a browser. |
* Update BpkNavigationBarButtonLink and Icon * snapshots --------- Co-authored-by: metalix2 <matthewr@skyscanner.net>
* Update BpkNavigationBarButtonLink and Icon * snapshots --------- Co-authored-by: metalix2 <matthewr@skyscanner.net>
Remember to include the following changes:
[KOA-123][BpkButton] Updating the colour
README.md
(If you have created a new component)README.md
.d.ts
) files updated