Skip to content
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

Merged
merged 45 commits into from
Apr 23, 2024
Merged

Conversation

narin
Copy link
Contributor

@narin narin commented Apr 15, 2024

Description of Change

Adds collapse/expand functionality to Alert.

  • Added the following props:
    • exapandable: determines whether alert is expandable or not
    • initializeExpanded: optional boolean that can be passed for when an alert needs to start off as expanded
  • Had to restructure the header a bit since we want the title text to be pressable as well in addition to the chevron icon. This caused some complexity in scenarios where a description is provided but a header is not. Added logic that determines the header text and wraps the header text either in a Pressable or an accessible View based on whether the Alert is collapsible and applies the correct accessibility labels and roles
  • Remove unused scrollTo and haptics code
  • Tested in mobile app (see video below)

Testing Packages

  • components@0.12.1-beta.2
  • components@0.12.1-beta.3
  • components@0.12.1-beta.4

Screenshots/Video

iOS expand/collapse

RPReplay_Final1713204302.MP4

iOS expand/collapse description only

RPReplay_Final1713204367.MP4

iOS voiceover

RPReplay_Final1713204588.MP4

iOS voiceover – description only

RPReplay_Final1713204729.MP4

Android

screen-20240415-111816.mp4

Web

Screen.Recording.2024-04-15.at.12.35.10.PM.mov

Web with chevron spacing

image

VA Mobile App address validation

RPReplay_Final1713209215.MP4

Testing

  • Tested on iOS
  • Tested on Android
  • Tested on Web
  • Tested in VA Mobile App

PR Checklist

Code reviewer validation:

  • General
    • PR is linked to ticket(s)
    • PR has changelog label applied if it's to be included in the changelog
    • Acceptance criteria:
      • All satisfied or
      • Documented reason for not being performed or
      • Split to separate ticket and ticket is linked by relevant AC(s)
    • Above PR sections adequately filled out
    • If any breaking changes, in accordance with the pre-1.0.0 versioning guidelines: a CU ticket has been created for the VA Mobile App detailing necessary adjustments with the package version that will be published by this ticket
  • Code
    • Tests are included if appropriate (or split to separate ticket)
    • New functions have proper TSDoc annotations

Publish

If changes warrant a new version per the versioning guidelines and the PR is approved and ready to merge:

@narin narin marked this pull request as ready for review April 15, 2024 19:50
@narin narin requested a review from a team as a code owner April 15, 2024 19:50
@TimRoe
Copy link
Contributor

TimRoe commented Apr 16, 2024

Had to restructure the header a bit since we want the title text to be pressable as well in addition to the chevron icon. This caused some complexity in scenarios where a description is provided but a header is not.

Probably should've been documented in the ticket or Figma, but I'd asked about whether an Alert could be collapsible in the Q&A thread in Slack and Jessica had responded:

If there is no header, the alert should not be collapsible.

So there's likely some cleanup around this case that can be done.

Edit:
Actually, looking again this was in the first AC of:

needs a header present and hides all content except the header when collapsed

but maybe not called out explicitly enough

{description && children ? <Spacer /> : null}
{children}
</View>
{expandable && <Spacer horizontal size={36} />}
Copy link
Contributor

@TimRoe TimRoe Apr 18, 2024

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).

Copy link
Contributor Author

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.

Copy link
Contributor

@TimRoe TimRoe Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying I need to add logic to set a max width for scaling?

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:

  • The entire Alert should be Pressable (we can move the chevron icon to be in the View row flex structure down here) or
  • We should use hitSlop to pad out the pressable area regardless of icon presence (again we can move the chevron icon to be in the View row flex structure down here)

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@narin narin requested a review from TimRoe April 19, 2024 18:53
Copy link
Contributor

@TimRoe TimRoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking better. A few minor things:

  • preventScaling on alert icon
  • Cleanup of the _header function a bit
  • Suggestion that maybe 'transparent' or 'none' fill on the spacerIcon might be more performant as it may potentially not actual bother fully rendering where I think giving it a color (even the same as below) might

Copy link
Contributor

@TimRoe TimRoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

Added 3 last minor suggestions to convert to the more web-like RN a11y prop names (accessibilityRole->role, accessibilityState->aria-expanded) since I assume (didn't check) Storybook web is complaining about the old versions being deprecated.

narin and others added 4 commits April 23, 2024 11:40
Co-authored-by: Tim R <TimRoe@users.noreply.github.com>
Co-authored-by: Tim R <TimRoe@users.noreply.github.com>
Co-authored-by: Tim R <TimRoe@users.noreply.github.com>
@narin narin merged commit ded5d3b into main Apr 23, 2024
6 of 7 checks passed
@narin narin deleted the feature/246-narin-alert-collapsible branch April 23, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants