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 32 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.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 retaining doing it this way, shouldn't the Spacer be 26?
10 Spacer + 16 wide chevron and then needing to scale 26->34 with font scaling (so I guess
10 + 16 * fontScale
+ some modifier to max it at 34).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.
Updated to 26. Are you saying I need to add logic to set a max width for scaling? I suppose another approach could be to add another invisible Icon with the same props as the chevron, but seems a little hacky.
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 want to mimic what the chevron is doing when font scaling comes into play, we'd need to match it growing but only a certain amount otherwise the description/children wrapping will get thrown off.
I think we'd be best suited to avoid using a Spacer here to account for the icon. From that other comment chain, I think either:
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
hitSlop
to expand coverage of the touchable area, but ran into a weird issue when moving the expand icon out of the Pressable. The icon itself wasn't responding to touches, but everywhere else within the hitSlop area was. Couldn't find too much about it on Google except that sometimes hitSlop doesn't work well with SVGs/icons, but strangely it works fine on the left Info icon. See video:screen-20240419-114524.mp4
Ended up just adding an invisible icon I've called
spacerIcon
which is guaranteed match the width of the expand icon at any scale. I tried using a Spacer instead and calculating the proper width with fontScale but it doesn't quite work since the icon and spacer scale at different rates due to the maxWidth.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.
Weird--but also something I think I may have run into once upon a time way back in my prior job of touchable weirdness with SVGs.
Unfortunate, but I think the solution makes sense given the odd circumstances.