Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Feature] Alert Collapse/Expand #288
[Feature] Alert Collapse/Expand #288
Changes from 24 commits
8bbbaed
f64e30d
721c440
91ccb43
3e09d1d
b5f67f3
bed831d
10bcdb6
f7c17bd
64c104e
b08aada
cf5c3f4
eafac84
09c4d9c
a6219ff
887b01a
edccedf
5c357f9
28eb3f1
66942fc
c07fdd5
aefcb0a
0f787b8
1f33742
19bfa38
e42ceee
49cd8bc
a2b5e9f
4e4d294
a28c2e1
99f1a6f
a604070
e2ebc33
fb28c2c
217fcec
6d1726d
58f6da8
cec6f69
5670a3e
cdf451f
0392eaa
27b88ca
bd0fef9
631ae25
d18b496
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Don't feel strongly since maybe it's clearer this way, but wouldn't this work just as well only using
let expanded = ...
and flipping the variable instead of useState?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.
Maybe worth checking with Jessica by bumping the Slack Q&A thread but based on Figma boxes for expandable Alert examples, it visually appears like the the chevron should also carve out space from the content so that text doesn't appear below the chevron when expanded and would wrap before being out that far.
If that is the case, I think this Icon needs to be moved below to be in the same View with
iconDisplay
below.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.
Added a Q&A in the thread for this while adding another somewhat related Q&A.
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.
Added a spacer to the right of the content to achieve this margin when expandable. Considered using flexbox for this, but it would have required me separating the single Pressable into 2 Pressables for the title and chevron. Thought it'd be better for the screenreader to just have the one.
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 think we are going to want to unify this in some capacity with the logic below. Can check with Jessica, but playing with the component currently the touch target for expand/collapse is currently only the header text (and trailing space) + the chevron. It doesn't include the alert icon (info/check/etc.) or space that's visually part of the collapsed Alert but not close enough to the text/chevron, leaving a fairly large area to the left end and the border on top/right/bottom sides that doesn't toggle the expand/collapse behavior.
Intuitively, I think tapping anywhere on the collapsed Alert should expand it--and similarly tapping anywhere along the top portion should collapse when expanded. Perhaps even further: when expanded tapping anywhere on the Alert that isn't an internal pressable (Buttons, children components with onPress) should also collapse it.
Basically: wondering if the entire Alert should be wrapped in the Pressable and we just have no onPress for non-expandable versions (and correspondingly hide the expand/collapse a11y stuff behind expandable).
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.
Confirmed with Jessica that the entire Alert should be pressable when it's collapsed. Will have to rework the structure so that header is separate from content.
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.
Alternatively, perhaps we use
hitSlop
with a Rect object to fill out the padding to be pressable? Doing it that way, we could also pull the Chevron out (to use flexbox like the alert icon) and ensure the entire "header" is pressable when open or closed. Not sure if that might get weird around font scaling with the hitSlop though.