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

tweak: Move alert icon to route pill #2828

Merged
merged 7 commits into from
Oct 3, 2024
Merged

Conversation

hannahpurcell
Copy link
Collaborator

Asana ticket: https://app.asana.com/0/1205385723132845/1208333281632130

Talked with Kathleen async and decided a few things that are not captured in the handoff doc (given that the doc was medium-fi):

  1. Make the route pills dynamically sized for the route ladders. (Rationale: if the 32/33 route had an alert, that's very wide. If we made all the pills that wide, it would look funny. So instead, give all pills 1.5rem padding.)
  2. Minimum margin around the routePill: .5rem.
  3. Alert + route number can be centered in the pill.

@hannahpurcell hannahpurcell requested a review from a team as a code owner September 27, 2024 20:06
@firestack
Copy link
Member

Hmm, I don't agree with moving the alert icon into the route pill component, can you instead make the route pill component take either a routeName OR children?

I'm imagining something like this

<RoutePill routeName={...} />
<RoutePill>
    {hasAlert && (
      <Tippy
        content="Active detour"
        trigger="click"
        onShow={() => tagManagerEvent("alert_tooltip_clicked")}
      >
        <div className="c-route-ladder__alert-icon" aria-label="Route Alert">
          <ExclamationTriangleFill />
        </div>
      </Tippy>
    )}
	<RoutePill.RouteName routeName={...} />
</RoutePill>

OR name the component which takes the children <RoutePill.Container/> and then make <RoutePill/> internally use <RoutePill.Container><RoutePill.RouteName/><RoutePill.Container/>

<RoutePill.Container>
    {hasAlert && (
      <Tippy
        content="Active detour"
        trigger="click"
        onShow={() => tagManagerEvent("alert_tooltip_clicked")}
      >
        <div className="c-route-ladder__alert-icon" aria-label="Route Alert">
          <ExclamationTriangleFill />
        </div>
      </Tippy>
    )}
	<RoutePill.RouteName routeName={...} />
</RoutePill>

@hannahpurcell
Copy link
Collaborator Author

Hmm, I don't agree with moving the alert icon into the route pill component, can you instead make the route pill component take either a routeName OR children?

Ahhh totally. I did it simply, without using .Container or .RouteName

Copy link
Member

@firestack firestack left a comment

Choose a reason for hiding this comment

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

one question but otherwise lgtm

@hannahpurcell hannahpurcell merged commit 2c26b55 into main Oct 3, 2024
45 checks passed
@hannahpurcell hannahpurcell deleted the hp/alert-icon-to-route-pill branch October 3, 2024 14:51
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