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

Tailwind 3 and Switch UI scaling from a base-10 to base-16 #1821

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

amazingphilippe
Copy link
Contributor

@amazingphilippe amazingphilippe commented Apr 26, 2024

Summary | Résumé

Notify is currently setup so that 1rem = 10px at 100% scaling. By default, 1rem = 16px at 100% scaling.

This is not a problem for us now. However, when we tried to import some design system components in the past, we did hit this conflict in UI scaling. The DS uses the default 16px base.

In theory, this should help us be in a better place to slowly implement the design system if we choose to do so.

This PR:

  • Changes our UI scaling from 10 to 16. This usually happens in the html and body CSS declarations. Ours was in style.css
  • Updated and cleaned up font size tokens.
  • Updated and cleaned up spacing tokens. The sizes were rounded off to nicer numbers. When we previously had a 2.5px token, I rounded it to 3px.

Test instructions | Instructions pour tester la modification

  • At the worst, there might be sub-pixel differences in our designs.
  • This should have no impact on anything. We might want to pool our efforts to go through the entire site.

Visual before and after of the size scale. Notice just some rounding for 2.5, 7.5 and 12.5
image

Copy link

@amazingphilippe amazingphilippe marked this pull request as ready for review April 26, 2024 14:37
Comment on lines +181 to +182
assert page.select("div[data-test-id='totals']")[0].text == str(sum(x[2] for x in stats))
assert page.select("div[data-test-id='services']")[0].text == str(len(services))
Copy link
Member

Choose a reason for hiding this comment

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

Love to see this instead of targetting css classes!

@@ -15,6 +15,7 @@ import { faCircleQuestion } from "@fortawesome/free-solid-svg-icons/faCircleQues
import { faTriangleExclamation } from "@fortawesome/free-solid-svg-icons/faTriangleExclamation";
import { faCircleExclamation } from "@fortawesome/free-solid-svg-icons/faCircleExclamation";
import { faInfoCircle } from "@fortawesome/free-solid-svg-icons";
import { faPaperPlane } from "@fortawesome/free-solid-svg-icons";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the scheduler updates

<div>
<Calendar />
</div>
<Calendar />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this unclassed div helped make the scheduler responsive don to 320px

<p>
<strong>
<p className="confirmationTime">
<time datetime={date}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a <time> element like in #1799

Comment on lines 88 to 98
.Calendar-item:focus:not(.Calendar-item--day):not(
.Calendar-item--unavailable
):not(:empty) {
outline: black inset;
outline-offset: -4px;
outline-offset: -4px;
outline: var(--color-focus) 3px solid;
}
.Calendar-item:hover:not(.Calendar-item--day):not(
.Calendar-item--unavailable
):not(:empty) {
background-color: var(--hover-color);
color: white;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One is hover. Two is focus. always takes me some time to distinguish those two selectors.

Comment on lines +108 to +109
gap: 4px;
padding: 2px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are designed to complement border widths, hence why I chose pixel instead of rem.

@@ -1,76 +1,81 @@
:root {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Github was hiding this file, but it contains quite a lot of manual changes.

In general:

  • Update local CSS variables.
  • Fix font sizes
  • Increased target size of the calendar dates
  • Map gutter and gutterHalf dimensions to the new CSS variables
  • Removed an entire component that was unused (toggle/switch)
  • Cleaned up Nav select and radios to match rest of Notify
  • Updated the confirmation box. (see screenshots)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

md and up
image
base
image

Copy link
Member

@andrewleith andrewleith left a comment

Choose a reason for hiding this comment

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

This looks great!

Couple of notes on the scheduler:

  • The enabled and disabled states of the month switch button are very hard to tell apart
  • The outline of the active day button is a bit faint (this is subjective, feel free to ignore)

@amazingphilippe
Copy link
Contributor Author

  • The enabled and disabled states of the month switch button are very hard to tell apart

Good point! I just noticed. Lets make this clearer (Good timing to test this behaviour)

  • The outline of the active day button is a bit faint (this is subjective, feel free to ignore)

+1, I can make this one darker too.

Copy link

github-actions bot commented Aug 2, 2024

@amazingphilippe amazingphilippe marked this pull request as draft September 16, 2024 14:21
Copy link

@amazingphilippe amazingphilippe marked this pull request as ready for review October 8, 2024 14:53
@amazingphilippe amazingphilippe changed the title Switch UI scaling from a base-10 to base-16 Tailwind 3 and Switch UI scaling from a base-10 to base-16 Oct 8, 2024
@@ -5,7 +5,7 @@
}

.big-number-number {
@apply block .text-titlelarge pb-4 font-bold leading-none;
@apply block text-titlelarge pb-4 font-bold leading-none;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just a typo we never caught until now. Tailwind 3 compiler caught this one

@@ -25,7 +25,7 @@

.button:focus,
a.button:focus {
outline: 3px solid theme("colors.yellow.default");
outline: 3px solid theme("colors.yellow.DEFAULT");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor syntax tweaks. tailwind expects an all caps DEFAULT it seems.

@@ -15,7 +15,7 @@
.fullscreen-content th,
.fullscreen-content .table-field-error-label,
.fullscreen-content .table-field-left-aligned {
@apply whitespace-no-wrap;
@apply whitespace-nowrap;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

another typo caught by tailwind 3 compiler

margin-left: -1.5rem;
margin-right: -1.5rem;
@apply my-0;
@apply my-0 -mx-gutterHalf;
Copy link
Contributor Author

@amazingphilippe amazingphilippe Oct 28, 2024

Choose a reason for hiding this comment

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

Here, I'm using our tokens, instead of using hard coded values.

@@ -148,11 +144,14 @@ section[id]:target {
box-decoration-break: clone;
}

.hidden,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tailwind reserves the hidden class. It does about the same thing.

.js-enabled .js-hidden {
@apply hidden invisible;
}

@layer utilities {
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 may have accidentally moved this up, but it addresses code on lines 204-209 below. From the migration guide: https://tailwindcss.com/docs/upgrade-guide#replace-variants-with-layer

@@ -347,15 +340,13 @@ label {
@apply p-2 max-w-full;
}

@screen smaller {
Copy link
Contributor Author

@amazingphilippe amazingphilippe Oct 28, 2024

Choose a reason for hiding this comment

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

There is no "smaller" breakpoint. These styles could apply to the base viewport sizes.

@@ -43,34 +40,55 @@ module.exports = {
md: "768px",
lg: "1024px",
},
spacing: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to theme the spacing, not extend base tailwind values. This means we move this into the theme section of the config

Comment on lines +27 to +34
xs: "0.875rem",
small: "1.0625rem",
smaller: "1.25rem",
base: "1.25rem",
title: "1.5rem",
titlelarge: "1.75rem",
lg: "2.25rem",
xxl: "3.25rem",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned up unused tokens. Next we can think of making the keys make sense.

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