A11y: Make Annotations and Template Variables list and edit pages responsive #71791
Conversation
juanicabanas
requested review from
polibb,
kaydelaney,
tskarhed and
eledobleefe
and removed request for
a team
| const value = useMemo( | ||
| () => REFRESH_OPTIONS.find((o) => o.value === refresh)?.value ?? REFRESH_OPTIONS[0].value, | ||
| [refresh] | ||
| ); | ||
|
|
||
| return ( | ||
| <Field label="Refresh" description="When to update the values of this variable"> | ||
| <RadioButtonGroup options={REFRESH_OPTIONS} onChange={onChange} value={value} /> | ||
| <RadioButtonGroup options={REFRESH_OPTIONS} onChange={onChange} value={value} size={width < 375 ? 'sm' : 'md'} /> |
There was a problem hiding this comment.
I don't think this is a good solution (not worth it, and just makes code messy, and not a pattern we use anywhere else )
There was a problem hiding this comment.
beside the radio button fits on small screens. I would change the wording instead "on time range change" to just "On time change"
There was a problem hiding this comment.
I'm also wondering how much responsive we should be since a 320px mobile device is not a common one anymore.
Changing the wording keeps generating some problems, since there's a minimal horizontal scrolling and no padding. Shouldn't we decrease the padding a little bit more, in this case?
|
Not sure how scrolling just the table is better than scrolling the page? is not scrolling the page a bit more discoverable? |
There was a problem hiding this comment.
I left small comments, but the changes look great. Thanks @juanicabanas to take care of this!
| </Droppable> | ||
| </DragDropContext> | ||
| </table> | ||
| <div style={{ overflow: 'scroll', width: '100%' }}> |
There was a problem hiding this comment.
Should we use here emotion as well?
There was a problem hiding this comment.
yes! I didn't catch it
| @@ -14,14 +15,15 @@ const REFRESH_OPTIONS = [ | |||
| ]; | |||
|
|
|||
| export function QueryVariableRefreshSelect({ onChange, refresh }: PropsWithChildren<Props>) { | |||
| const { width } = useWindowSize(); | |||
There was a problem hiding this comment.
Should we use useMediaQueryChange hook? This will only trigger a re-render when the media query changes, not when the window size changes.
There was a problem hiding this comment.
loved it. didn't know it
| const value = useMemo( | ||
| () => REFRESH_OPTIONS.find((o) => o.value === refresh)?.value ?? REFRESH_OPTIONS[0].value, | ||
| [refresh] | ||
| ); | ||
|
|
||
| return ( | ||
| <Field label="Refresh" description="When to update the values of this variable"> | ||
| <RadioButtonGroup options={REFRESH_OPTIONS} onChange={onChange} value={value} /> | ||
| <RadioButtonGroup options={REFRESH_OPTIONS} onChange={onChange} value={value} size={width < 375 ? 'sm' : 'md'} /> |
There was a problem hiding this comment.
Out of curiosity, why don't we set a width and the labels are automatically truncated? I like this improvement anyways, although md works for screens like iPhone SE already.
There was a problem hiding this comment.
I just tried it and it seems it's not possible with the RadioButtonGroup we currently have. I should make some modifications to that core component
@torkelo I think it's better since otherwise, the whole page would look as broken |
|
@juanicabanas yea, your right. just feels like this just hides the problem but maybe with more rows in the table it's easier to notice |
ivanortegaalba
deleted the
juanicabanas/responsive-config-dashboard-section-a11-bug
branch
What is this feature?
It makes annotations, template variables and edit template variables configuration pages responsive.
Which issue(s) does this PR fix?:
Fixes #66487
Special notes for your reviewer:
Since there are tables inside these pages, it's not possible to avoid the horizontal scrolling behavior.
I fixed the responsive design.
Annotations config page
annotations_page.mov
Template variables config page
variables_page.mov
Template variables edit page
variables_edit_page.mov
Please check that: