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

fix: Allow line wrapping on flex zone alerts. #2094

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

cmaddox5
Copy link
Contributor

Asana task: ad-hoc

When I reviewed #2068, I didn't think about flex zone alerts. Because of the limited space on those, we should allow station names to wrap. Tweaked function a bit so that it returns a string for flex zone alerts.

  • Tests added?

@cmaddox5 cmaddox5 requested a review from a team as a code owner June 27, 2024 15:13
Copy link

Coverage of commit 90fdb18

Summary coverage rate:
  lines......: 45.0% (2916 of 6482 lines)
  functions..: 41.0% (1039 of 2532 functions)
  branches...: no data found

Files changed coverage rate:
                                                                          |Lines       |Functions  |Branches    
  Filename                                                                |Rate     Num|Rate    Num|Rate     Num
  ==============================================================================================================
  lib/screens/v2/widget_instance/reconstructed_alert.ex                   |86.8%    340| 100%    45|    -      0

Download coverage report

@digitalcora
Copy link
Contributor

digitalcora commented Jun 27, 2024

Perhaps you've already thought about this, but could it make sense to handle this with CSS? In other words, always apply the no-wrap spans, but conditionally ignore them in the styles scoped to the appropriate components. This feels like it would be a less complex approach than having two different ways of generating the data on the server, and matches a presentation-level issue with a presentation-level fix.

@cmaddox5
Copy link
Contributor Author

@digitalcora The difficulty with that is preventing line breaks between issue, location, and cause. The flexzone component assumes that location is always a string so is it able to display the three in a single fragment with string concatenation. Introducing FreeText adds a div which means that location is always on its own line even without unsetting white-space on the station names. I do see a lot of room for improvement in the way we render reconstructed_alert and reconstructed_takeover (mixing string and FreeTextType in the same prop is something we never should have done), but due to the time sensitive nature of the task it might be best to revisit after fixing the bug.

Copy link
Contributor

@digitalcora digitalcora left a comment

Choose a reason for hiding this comment

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

🉑

@digitalcora
Copy link
Contributor

Can you tell off-hand whether this would resolve this crash on pre-fare audio readouts?

@cmaddox5
Copy link
Contributor Author

Can you tell off-hand whether this would resolve this crash on pre-fare audio readouts?

Hmm, that may actually be a separate issue. The visuals are fine for that alert, but the audio view isn't expecting the location to be anything but a string.

@digitalcora
Copy link
Contributor

Right, I thought since you changed this to an approach of only providing a FreeText value when specifically requested, that it might be a plain string once again for the audio view.

@cmaddox5
Copy link
Contributor Author

That change was focused specifically on flex zone alerts. The alert showing on that screen is full screen, which was what the original task was meant for. There looks to be a missing piece to the audio view for single screen alerts after the original change. I'm willing to own fixing that issue as well since I missed it in my review. Can start that after merging this.

@cmaddox5 cmaddox5 merged commit 9ffd4b4 into main Jun 28, 2024
13 checks passed
@cmaddox5 cmaddox5 deleted the cm/pre-fare-alert-flex-zone-location-fix branch June 28, 2024 16:12
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.

2 participants