-
Notifications
You must be signed in to change notification settings - Fork 19
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
Upgrade Salt and replace components #618
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
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.
Pretty sure this isn't used
@@ -13,10 +13,10 @@ test('renders a filter dropdown', async () => { | |||
</ToolbarProvider> | |||
); | |||
// assert | |||
expect(getAllByRole('option').length).toEqual(1); | |||
expect(getAllByRole('combobox').length).toEqual(1); |
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.
DD no longer has a nested option as a label
/** Callback to translate the selected list item to a Button label */ | ||
labelButton?: (selectedItems: string[] | undefined) => string; | ||
/** Dropdown list source */ | ||
source?: string[]; | ||
itemToString?: DropdownProps['valueToString']; |
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.
Backwards compatible
|
||
export default { | ||
root: style({ | ||
display: 'contents' |
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.
Fixes icons not being centered vertically
import { ToolbarButton } from '../Toolbar/ToolbarButton'; | ||
|
||
interface SaveAdornmentProps { | ||
onSave: ButtonProps['onClick']; | ||
} | ||
|
||
export const SaveAdornment = ({ onSave }: SaveAdornmentProps) => ( | ||
<StaticInputAdornment> |
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.
Was missed in a previous update, Input no longer needs this
@@ -134,18 +133,16 @@ export const PersistDialog = ({ meta, persistUrl }: PersistDialogProps) => { | |||
)} | |||
</DialogContent> | |||
<DialogActions> | |||
<ButtonBar> |
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.
ButtonBar doesn't work with normal buttons
@@ -52,24 +50,16 @@ export const AppHeaderControls: React.FC = () => { | |||
}); | |||
} | |||
if (isLoginEnabled && isLoggedIn) { | |||
actionMenuOptions = [...actionMenuOptions, { title: 'Logout', link: '/api/auth/signout' }]; |
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.
Lots of this logic wasn't used and the options don't seem extensible.
> | ||
<Icon aria-label="select an action" name="microMenu" /> | ||
</MenuButton> | ||
<Menu placement="bottom-end"> |
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.
Has a nice side-effect of fixing menu appearing behind the app header
@@ -7,7 +7,8 @@ import { ExpansionIcon } from './ExpansionIcon'; | |||
import { useWindow } from '@salt-ds/window'; | |||
import { useComponentCssInjection } from '@salt-ds/styles'; | |||
|
|||
import navigationItemCss from '@salt-ds/core/dist-es/navigation-item/NavigationItem.css.js'; | |||
const navigationItemCss = |
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.
Had to inline the CSS, this worked because core was bundled into site-components, but that broke using other components. Adding core to the package.json and inlining this was the easiest way to fix it.
c5d0976
to
0b4543d
Compare
0b4543d
to
7ba59ac
Compare
/release-pr |
🫰✨ Thanks @joshwooding! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add @jpmorganchase/[email protected] yarn add in 17.69s. |
No description provided.