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

Use Codex tokens and custom spacing variables #762

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

codders
Copy link
Contributor

@codders codders commented Sep 20, 2024

Migrate away from Wikit tokens to Codex SASS variables where they are available. In some cases we use custom CSS variables for values from Wikit whose Codex equivalents are missing.

Bug: T369510
Bug: T369508
Bug: T369509

@codders codders changed the base branch from main to codex-button September 20, 2024 07:20
@codders codders force-pushed the feat/codex-tokens-and-spacing-vars-20240920 branch from 6b0162c to 41e42f3 Compare September 20, 2024 07:28
Base automatically changed from codex-button to main September 20, 2024 08:13
@codders codders force-pushed the feat/codex-tokens-and-spacing-vars-20240920 branch from 41e42f3 to d13bac4 Compare September 20, 2024 08:38
@lucaswerkmeister
Copy link
Member

Is there a reason to make --dimension-layout-xsmall a CSS variable? Looking at this change overall, to me it feels like it would make more sense to make it a Sass variable like all the other tokens. (This would also fix the minor issue that the current version includes the variable styles three times in the build output as part of different components.)

Copy link
Member

@lucaswerkmeister lucaswerkmeister left a comment

Choose a reason for hiding this comment

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

Tried to go through the changes and figure out which task they correspond to and whether they’re right or not. Many of the comments are just acknowledgements, but I think some represent issues.

src/components/LanguageInput.vue Show resolved Hide resolved
src/components/NewLexemeForm.vue Outdated Show resolved Hide resolved
src/components/NewLexemeForm.vue Show resolved Hide resolved
src/components/NewLexemeForm.vue Show resolved Hide resolved
src/components/NewLexemeForm.vue Show resolved Hide resolved
src/components/NewLexemeForm.vue Show resolved Hide resolved
src/components/RequiredAsterisk.vue Outdated Show resolved Hide resolved
src/components/SearchExisting.vue Outdated Show resolved Hide resolved
src/components/SearchExisting.vue Show resolved Hide resolved
@codders codders force-pushed the feat/codex-tokens-and-spacing-vars-20240920 branch from d13bac4 to a9d7f41 Compare September 23, 2024 08:12
@codders
Copy link
Contributor Author

codders commented Sep 23, 2024

Is there a reason to make --dimension-layout-xsmall a CSS variable? Looking at this change overall, to me it feels like it would make more sense to make it a Sass variable like all the other tokens. (This would also fix the minor issue that the current version includes the variable styles three times in the build output as part of different components.)

I'm not super opinionated. I think the intention was to have a CSS file at the end of the migration that we could try an upstream to Codex to have them add the variables to the CSS, but we could also keep the missing variables in Sass in our repo for now.

@lucaswerkmeister
Copy link
Member

I'm not super opinionated. I think the intention was to have a CSS file at the end of the migration that we could try an upstream to Codex to have them add the variables to the CSS, but we could also keep the missing variables in Sass in our repo for now.

I’d prefer to make them Sass variables for now and then potentially reevaluate that at the end of T375370, but I’m also okay with keeping them as they are.

(The other questions are now blocked on feedback from UX IIUC.)

Migrate away from Wikit tokens to Codex SASS variables where
they are available. In some cases we use custom CSS variables
for values from Wikit whose Codex equivalents are missing.

Bug: T369510
Bug: T369508
Bug: T369509
@codders codders force-pushed the feat/codex-tokens-and-spacing-vars-20240920 branch from a9d7f41 to 030fc4b Compare October 11, 2024 07:40
@codders
Copy link
Contributor Author

codders commented Oct 11, 2024

Updated per UX feedback - should be good to go now.

@lucaswerkmeister
Copy link
Member

There’s currently too much space before the lexeme language / lexical category asterisks, but I think that’s just a consequence of #773 being blocked – the asterisks currently have both the gap on the surrounding label and the margin that’s supposed to replace the gap, if I understand correctly.
image

@codders codders merged commit 71a7cd5 into main Oct 11, 2024
3 checks passed
@codders codders deleted the feat/codex-tokens-and-spacing-vars-20240920 branch October 11, 2024 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants