-
Notifications
You must be signed in to change notification settings - Fork 623
Mobile UI: search (drawer) #2855
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
base: main
Are you sure you want to change the base?
Conversation
5a82af7
to
be785bc
Compare
textStyle: 'lg', | ||
fontWeight: 'semibold', | ||
textStyle: 'heading.md', | ||
lineHeight: '32px', |
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.
I believe the line height should remain at the default value, but the minimum height for the header should be set. Additionally, there is no need to define the font weight here, as it is already specified in the typography settings.
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.
if I remove font weight property, the title gets 600 (semibold), and I can't find the cause of that problem
onSubmit={ voidFn } | ||
onFocus={ voidFn } | ||
onHide={ voidFn } | ||
onBlur={ voidFn } |
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.
If we don't need these handlers in some scenarios, it might be better to make those props optional.
onClear={ handleClear } | ||
onFormClick={ onTriggerClick } | ||
value={ searchTerm } | ||
mt={ -5 } |
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.
Can we redefine the top padding of the drawer body instead of applying a negative margin here?
<DrawerFooter | ||
borderTop="1px solid" | ||
borderColor="border.divider" | ||
bg="background.primary" |
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.
What is the reason for changing the background here? It seems we don't have that token at the moment.
const actionBar = document.querySelector('[data-testid="action-bar"]') || | ||
document.querySelector('[class*="ActionBar"]') || | ||
document.querySelector('[class*="action-bar"]'); |
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 this code correct? I cannot find any occurrences of the ActionBar class name or the action-bar test ID.
|
||
// Check for TableHeaderSticky - it should not be stuck when at top | ||
await page.waitForFunction(() => { | ||
const stickyHeaders = document.querySelectorAll('[class*="TableHeaderSticky"]'); |
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.
Same comment here.
And not all table headers are stuck at the top of the page.
Description and Related Issue(s)
resolves #2837
Checklist for PR author