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

feat(connected-itinerary-body): make alert collapse configurable #1182

Merged
merged 5 commits into from
Mar 28, 2024

Conversation

amy-corson-ibigroup
Copy link
Contributor

@amy-corson-ibigroup amy-corson-ibigroup commented Mar 26, 2024

Description:
Use alwaysCollapseAlerts prop in connected itinerary body to configure whether users can collapse alerts.

PR Checklist:

  • Does the code follow accessibility standards (WCAG 2.1 AA Compliant)?
  • Are all languages supported (Internationalization/Localization)?
  • Are appropriate Typescript types implemented?

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

One small nitpick!

@@ -347,6 +347,8 @@ itinerary:
renderRouteNamesInBlocks: true
# Whether the mode icons should be colored as well
fillModeIcons: true
# Allow user to collapse alerts in itinerary body
collapseAlerts: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is misleadingly named. Can we name it allowUserAlertCollapsing?

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

Looks good! Just one boolean thing to deal with

@@ -116,6 +117,7 @@ class ConnectedItineraryBody extends Component {
<ItineraryBodyContainer>
<StyledItineraryBody
accessibilityScoreGradationMap={accessibilityScoreGradationMap}
alwaysCollapseAlerts={allowUserAlertCollapsing}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
alwaysCollapseAlerts={allowUserAlertCollapsing}
alwaysCollapseAlerts={!allowUserAlertCollapsing}

Should this be flipped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, I'm setting allowUserAlertCollapsing to true in the config!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless we want alerts to collapse by default, which might be preferable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

But as it is right now if allowUserAlertCollapsing is true, then they won't be allowed to collapse them right? Or am I forgetting how the behavior works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, alwaysCollapseAlerts needs to be true for the alerts to collapse

Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

Please add the new attribute to the corresponding config type.

Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

@amy-corson-ibigroup amy-corson-ibigroup merged commit 1d015c8 into dev Mar 28, 2024
9 checks passed
@amy-corson-ibigroup amy-corson-ibigroup deleted the make-alerts-collapseable-config branch March 28, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants