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

Jump to bottom

feat: Add cell borders and colors for active detour table #2788

Merged
merged 3 commits into from
Sep 24, 2024
Merged

feat: Add cell borders and colors for active detour table #2788

merged 3 commits into from
Sep 24, 2024

Conversation

joshlarson
Copy link
Contributor

@joshlarson joshlarson commented Sep 11, 2024 •
edited
Loading

@joshlarson joshlarson requested a review from a team as a code owner September 11, 2024 18:32
@joshlarson joshlarson force-pushed the jdl/feat/icons-for-detour-table-headers branch from 3bdd54b to ad664ca Compare September 11, 2024 18:52
@joshlarson joshlarson force-pushed the jdl/feat/active-detour-table-cell-borders-and-colors branch from 980407d to 1015c4f Compare September 11, 2024 18:54
@github-actions GitHub Actions
Copy link

Coverage of commit 1015c4f

Summary coverage rate:
  lines......: 93.0% (3318 of 3567 lines)
  functions..: 72.5% (1364 of 1882 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@joshlarson joshlarson force-pushed the jdl/feat/icons-for-detour-table-headers branch from ad664ca to 01772ae Compare September 11, 2024 21:07
@joshlarson joshlarson force-pushed the jdl/feat/active-detour-table-cell-borders-and-colors branch from 1015c4f to eaec76b Compare September 11, 2024 21:23
@github-actions GitHub Actions
Copy link

Coverage of commit eaec76b

Summary coverage rate:
  lines......: 93.0% (3318 of 3567 lines)
  functions..: 72.5% (1364 of 1882 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@joshlarson joshlarson force-pushed the jdl/feat/icons-for-detour-table-headers branch from 01772ae to b028c5e Compare September 13, 2024 18:18
@joshlarson joshlarson force-pushed the jdl/feat/active-detour-table-cell-borders-and-colors branch from eaec76b to 0070b45 Compare September 13, 2024 18:18
@github-actions GitHub Actions
Copy link

Coverage of commit 0070b45

Summary coverage rate:
  lines......: 93.0% (3317 of 3567 lines)
  functions..: 72.4% (1363 of 1882 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

firestack
firestack reviewed Sep 19, 2024
View reviewed changes
assets/css/detours/_detour_table.scss Outdated
Comment on lines 72 to 81
&.c-detours-table--active {
td {
background-color: tokens.$lemon-100;
border-color: tokens.$lemon-600 !important;
}

th {
border-bottom-color: tokens.$lemon-600 !important;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

question(slightly-non-blocking): I was hoping we'd be able to do all of this with bootstrap, are we writing the CSS here because we don't have the bootstrap reboot yet? or because the colors in ui-alert don't match "exactly" what the designs used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should pair on this? I didn't put a huge amount of time into trying it, but I couldn't figure out how to create new bootstrap utility classes. So bg-primary works and applies a background to the cells, but bg-lemon-600 or bg-active-detour don't seem to do anything. I tried updating the theme colors in both the color definitions and the bootstrap variable overrides, but neither of those changes had any effect, so I gave up.

Also at least my first attempt with bootstrap utilities led to a bit of prop-drilling, which I didn't love.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah let's pair and see what we can find.

firestack
firestack requested changes Sep 19, 2024
View reviewed changes
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.

marking as changes requested to get github to show better information on the PR's overview

@firestack firestack force-pushed the jdl/feat/active-detour-table-cell-borders-and-colors branch from 0070b45 to 97862df Compare September 19, 2024 16:06
@joshlarson joshlarson force-pushed the jdl/feat/icons-for-detour-table-headers branch 2 times, most recently from 3e061f5 to 001369e Compare September 23, 2024 15:57
@joshlarson joshlarson force-pushed the jdl/feat/active-detour-table-cell-borders-and-colors branch from 97862df to 056dcfd Compare September 23, 2024 15:57
@github-actions GitHub Actions
Copy link

Coverage of commit 056dcfd

Summary coverage rate:
  lines......: 92.5% (3302 of 3568 lines)
  functions..: 71.8% (1365 of 1901 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@joshlarson joshlarson force-pushed the jdl/feat/icons-for-detour-table-headers branch from 001369e to a0bfbe0 Compare September 23, 2024 18:48
@joshlarson joshlarson force-pushed the jdl/feat/active-detour-table-cell-borders-and-colors branch from 056dcfd to b03748f Compare September 23, 2024 18:48
@joshlarson joshlarson force-pushed the jdl/feat/icons-for-detour-table-headers branch from a0bfbe0 to 068cc76 Compare September 24, 2024 09:07
Base automatically changed from jdl/feat/icons-for-detour-table-headers to main September 24, 2024 13:53
@joshlarson joshlarson force-pushed the jdl/feat/active-detour-table-cell-borders-and-colors branch 2 times, most recently from 6fd418e to 897b495 Compare September 24, 2024 16:21
@joshlarson joshlarson force-pushed the jdl/feat/active-detour-table-cell-borders-and-colors branch from 5099eba to 6241f2a Compare September 24, 2024 16:29
@joshlarson
Copy link
Contributor Author

@firestack Thanks for pairing! Want to take another look now?

firestack
firestack approved these changes Sep 24, 2024
View reviewed changes
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 small thing but otherwise looks good 👍🏻

assets/src/components/detoursTable.tsx Outdated
Comment on lines 44 to 48
className={joinClasses([
...classNames,
"c-detours-table",
status === DetourStatus.Active ? "table-active-detour" : null,
])}
Copy link
Member

Choose a reason for hiding this comment

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

suggestion(non-blocking if not small): is it possible to use the variant prop 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.

That does seem to have worked

@github-actions GitHub Actions
Copy link

Coverage of commit 6241f2a

Summary coverage rate:
  lines......: 92.5% (3310 of 3580 lines)
  functions..: 71.4% (1366 of 1912 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@joshlarson joshlarson enabled auto-merge (squash) September 24, 2024 18:43
@joshlarson joshlarson merged commit 1154994 into main Sep 24, 2024
9 checks passed
@joshlarson joshlarson deleted the jdl/feat/active-detour-table-cell-borders-and-colors branch September 24, 2024 18:49
@github-actions GitHub Actions
Copy link

Coverage of commit 7dfe67b

Summary coverage rate:
  lines......: 92.4% (3309 of 3580 lines)
  functions..: 71.4% (1365 of 1912 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@firestack firestack firestack approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@joshlarson @firestack