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.

Sign up for GitHub

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: Updated route-pill with a new size, currently called "large-format" #2600

Merged
merged 5 commits into from
May 20, 2024
Merged

feat: Updated route-pill with a new size, currently called "large-format" #2600

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
edbcefc
Updated route-pill with a new size, currently called "large-format"
hannahpurcell May 16, 2024
d590547
formatting
hannahpurcell May 16, 2024
e0043b3
RoutePill story
hannahpurcell May 16, 2024
6495a57
Added story to assets/stories/skate-components/routePill.stories.tsx
hannahpurcell May 16, 2024
08c065c
Adjusted className to a largeFormat boolean
hannahpurcell May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions assets/css/_route_pill.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
.c-route-pill {
min-width: 2.875rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh 🤔

max-width: 2.875rem;
width: 2.875rem;
height: 1.5rem;

border-radius: 0.8125rem;
Expand All @@ -10,6 +9,18 @@
line-height: 1.5rem;
font-weight: 700;
font-family: $font-family-route-pill;

display: flex;
flex-direction: column;
justify-content: center;
Comment on lines +13 to +15
Copy link
Member

Choose a reason for hiding this comment

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

question(non-blocking): why the switch to flex?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After expanding the route pill width/height, I needed a way to get the text centered, and flex is what always comes to mind for positioning for me. But I also realize, I could instead expand line-height for the text to be the height of the route-pill, so line-height:2.25rem. (I didn't think of that since the Figma specified line-height 1.5rem, but it's a value that doesn't have impact.) Is this preferable?

.c-route-pill {
  width: 2.875rem;
  height: 1.5rem;

  border-radius: 0.8125rem;

  text-align: center;
  font-size: 1rem;
  line-height: 1.5rem;
  font-weight: 700;
  font-family: $font-family-route-pill;

  &--large-format {
    width: 4.5rem;
    height: 2.25rem;
    line-height: 2.25rem;
    border-radius: 1.875rem;
    font-size: $h3-font-size;
    font-weight: 600;
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Would line-height: 1.5 work? That way it would auto-adjust based on the font size

https://developer.mozilla.org/en-US/docs/Web/CSS/line-height#number

Copy link
Collaborator Author

@hannahpurcell hannahpurcell May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The height to font-size ratio is not consistent for both versions of this button. So font-size:height for the default version is 1:1.5 while font-size:height for the large format is 1.25:2.25 or 1:1.8

EDIT: so line-height cannot be simplified to one ratio or the other

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh - I had thought it was 1.5 : 2.25 and 1 : 1.5, which would have been consistent.

I'll back out of this conversation then, since I don't have an opinion on this (I would probably have gravitated towards flex too, fwiw)


&--large-format {
width: 4.5rem;
height: 2.25rem;
border-radius: 1.875rem;
font-size: $h3-font-size;
font-weight: 600;
}
}

.c-route-pill--bus {
Expand Down
2 changes: 1 addition & 1 deletion assets/src/components/detours/diversionPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export const DiversionPanel = ({

<div className="d-flex">
<RoutePill
className="d-inline-block me-2 align-top"
className="me-2 align-top"
routeName={routeName}
/>

Expand Down
Loading