[LOOM-1300][BpkModalInner] Style 2 fixes #3416
Conversation
|
Visit https://backpack.github.io/storybook-prs/3416 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3416 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3416 to see this build running in a browser. |
| margin-right: -1 * $button-padding; | ||
| margin-left: -1 * $button-padding; |
There was a problem hiding this comment.
We always set a margin on the button when it's not always wanted. We overrode this in the BpkModal here
There was a problem hiding this comment.
It's best to leave the button without a margin and allow the other componets to add one if needed.
|
Visit https://backpack.github.io/storybook-prs/3416 to see this build running in a browser. |
| justify-content: center; | ||
|
|
||
| &--icon { | ||
| width: tokens.bpk-spacing-lg(); |
There was a problem hiding this comment.
In figma the icon is expected to have a "Hug" of 24px Padding increasing the icon area from 16 to 24px. Though the hug is not expected on a Link that is passed.
| @@ -107,7 +111,10 @@ const BpkNavigationBar = (props: Props) => { | |||
| className={getClassNames( | |||
| 'bpk-navigation-bar__trailing-item', | |||
| `bpk-navigation-bar__trailing-item-${barStyle}`, | |||
| // if the Button has children safe to assume it's not an icon | |||
| !trailingButton.props?.children && 'bpk-navigation-bar__trailing-item--icon' | |||
There was a problem hiding this comment.
The trailingButton prop isn't necessary an Icon it can be a Link and in that case we shouldn't set the icon style which adds the "Hug".
There was a problem hiding this comment.
Perhaps the prop should be renamed to trailingItem like the Styling is? For another PR perhaps.
| &--justified { | ||
| justify-content: space-between; | ||
| } | ||
|
|
There was a problem hiding this comment.
For modals where the title is not centred we should justify the title to fill up the whole nav bar.
|
Some expected visual changes. Fixed the NavBar height it was 0.5px to high this impacts the datepicker calendar. and Other snapshots
|
|
Visit https://backpack.github.io/storybook-prs/3416 to see this build running in a browser. |
| &__navigation { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| } | ||
|
|
There was a problem hiding this comment.
Moved to the Navigation Component and created a justifed prop.
There was a problem hiding this comment.
We already have justification in side the Navigation Component. Safe to remove.
| &__close-button { | ||
| position: relative; // Override position: absolute coming from BpkNavigationBar | ||
| right: auto; | ||
|
|
||
| @include margins.bpk-margin-leading(tokens.bpk-spacing-base()); | ||
|
|
||
| @include utils.bpk-rtl { | ||
| left: auto; | ||
| } | ||
|
|
||
| &-style--surface-contrast { | ||
| background-color: tokens.$bpk-surface-contrast-day; | ||
| color: tokens.$bpk-text-primary-dark-color; | ||
|
|
||
| @include utils.bpk-hover { | ||
| color: tokens.$bpk-text-primary-dark-color; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The Nav and Button components already do all this for us.
|
Visit https://backpack.github.io/storybook-prs/3416 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3416 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3416 to see this build running in a browser. |
| @@ -22,13 +22,7 @@ | |||
| $button-padding: tokens.bpk-spacing-sm() * 0.5; | |||
|
|
|||
| .bpk-close-button { | |||
| width: tokens.bpk-spacing-lg(); | |||
There was a problem hiding this comment.
Another option is to add a prop to include the "Hug" margin.
|
Visit https://backpack.github.io/storybook-prs/3416 to see this build running in a browser. |
| &--justified { | ||
| justify-content: space-between; | ||
| } |
There was a problem hiding this comment.
Yes! good spot removed 😅
| @include typography.bpk-label-2; | ||
|
|
||
| &--icon { | ||
| width: tokens.bpk-spacing-lg(); |
There was a problem hiding this comment.
Why do we need this? Don't all the items in the bar need to be spaced out in the same way? Like why would the gap between icon --> edge be less/more than word --> edge?
There was a problem hiding this comment.
We could remove this and have the consumers setting the icon, include it's own wrapper and classname to add width and padding.
There was a problem hiding this comment.
If its an icon we set a width to create padding around the Icon as shown in figma:
|
Visit https://backpack.github.io/storybook-prs/3416 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3416 to see this build running in a browser. |
|
The main visual changes is due to fixing the nav bar height in this PR: (16 padding + 24 line-height + 16 padding = 56) PR: |
Some Styling fixes to match the Designs and simplify how the styles are applied.
Remember to include the following changes:
[KOA-123][BpkButton] Updating the colourREADME.md(If you have created a new component)README.md.d.ts) files updated