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

fix(*): define missing custom properties from theme directory #3476

Merged
merged 17 commits into from
Jan 16, 2025

Conversation

cdransf
Copy link
Member

@cdransf cdransf commented Jan 3, 2025

Description

CSS-1084

  • fixes missing custom properties by transferring them from s2-foundations-redux

image
image

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • I have tested these changes in Windows High Contrast mode.
  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

@cdransf cdransf added the size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. label Jan 3, 2025
@cdransf cdransf self-assigned this Jan 3, 2025
Copy link

changeset-bot bot commented Jan 3, 2025

🦋 Changeset detected

Latest commit: 557d0f5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@spectrum-css/infieldbutton Patch
@spectrum-css/actionbutton Patch
@spectrum-css/clearbutton Patch
@spectrum-css/avatar Patch
@spectrum-css/button Patch
@spectrum-css/dialog Patch
@spectrum-css/preview Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Jan 3, 2025

🚀 Deployed on https://pr-3476--spectrum-css.netlify.app

Copy link
Contributor

github-actions bot commented Jan 3, 2025

File metrics

Summary

Total size: 1.74 MB*
Total change (Δ): 🔴 ⬆ 2.57 KB (0.14%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package Size Δ
actionbutton 23.32 KB 🔴 ⬆ 1.10 KB
avatar 6.55 KB 🟢 ⬇ 0.05 KB
button 29.96 KB 🔴 ⬆ 0.23 KB
clearbutton 6.21 KB 🔴 ⬆ 0.57 KB
dialog 15.04 KB 🟢 ⬇ 0.05 KB
infieldbutton 15.59 KB 🔴 ⬆ 0.80 KB

Details

actionbutton

Filename Head Compared to base
index.css 23.32 KB 🔴 ⬆ 1.10 KB (4.93%)

avatar

Filename Head Compared to base
index.css 6.55 KB 🟢 ⬇ 0.05 KB (-0.75%)

button

Filename Head Compared to base
index.css 29.96 KB 🔴 ⬆ 0.23 KB (0.77%)

clearbutton

Filename Head Compared to base
index.css 6.21 KB 🔴 ⬆ 0.57 KB (9.92%)

dialog

Filename Head Compared to base
index.css 15.04 KB 🟢 ⬇ 0.05 KB (-0.32%)

infieldbutton

Filename Head Compared to base
index.css 15.59 KB 🔴 ⬆ 0.80 KB (5.26%)
* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@cdransf cdransf force-pushed the cdransf/undefined-token-fixes-css-1084 branch from d72049e to 1303a5e Compare January 3, 2025 22:10
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

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

Nice work on this! I just wanted to double check with you on some of those action button "unused custom properties" (I left a comment). Otherwise, I didn't get any linter warnings/errors for each of the components ✅

components/actionbutton/index.css Show resolved Hide resolved
components/actionbutton/index.css Outdated Show resolved Hide resolved
components/actionbutton/index.css Outdated Show resolved Hide resolved
@cdransf cdransf added skip_vrt Add to a PR to skip running VRT (but still pass the action) ready-for-review labels Jan 3, 2025
@cdransf cdransf force-pushed the cdransf/undefined-token-fixes-css-1084 branch 3 times, most recently from 2f3fc28 to 16ddbc6 Compare January 6, 2025 17:10
Copy link
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

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

Wow, this ended up being quite the collection of components, especially with actionbutton and dialog in the mix! It looks like you took care of all the no undefined custom props warnings, so that's great!

I think there are a few remaining oddities particularly with actionbutton, like the selected state content color (and also the default/disabled state border, which I didn't comment on within the code because I wasn't able to figure out if that was because of a missing/undefined custom prop or something else). Hopefully we can resolve them here, or if not, in the S2 migration. I'd also noticed that when I was running yarn build on this branch that there were some lingering metadata changes that needed to be committed, but that may or may not still be an issue if your branch has been updated since then.

components/dialog/index.css Show resolved Hide resolved
components/actionbutton/index.css Show resolved Hide resolved
@cdransf
Copy link
Member Author

cdransf commented Jan 6, 2025

Wow, this ended up being quite the collection of components, especially with actionbutton and dialog in the mix! It looks like you took care of all the no undefined custom props warnings, so that's great!

I think there are a few remaining oddities particularly with actionbutton, like the selected state content color (and also the default/disabled state border, which I didn't comment on within the code because I wasn't able to figure out if that was because of a missing/undefined custom prop or something else). Hopefully we can resolve them here, or if not, in the S2 migration. I'd also noticed that when I was running yarn build on this branch that there were some lingering metadata changes that needed to be committed, but that may or may not still be an issue if your branch has been updated since then.

I left a few comments, fixed the actionbutton selected state and updated the metadata ✨

@cdransf cdransf force-pushed the cdransf/undefined-token-fixes-css-1084 branch from 4341683 to b4c0701 Compare January 6, 2025 20:41
Copy link
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

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

Hi again! Found a few more selected states in action button that likely need fixing, and I'm bringing up a hover question/issue with the static color variant for clear button.

components/actionbutton/index.css Show resolved Hide resolved
components/clearbutton/index.css Show resolved Hide resolved
@rise-erpelding
Copy link
Collaborator

rise-erpelding commented Jan 7, 2025

One more thing! Those dual-colored borders in action button are happening because the border-style is no longer being set! In s2 foundations we're setting it by importing the commons/base-button styles.

Therefore in computed border styles we end up with an outset border style in spectrum-two:
image

But what we have in s2 foundations is a solid border style:
image

It looks like button is still importing the base button styles in spectrum-two, maybe we can add this import line back to action button here as well?

@cdransf cdransf force-pushed the cdransf/undefined-token-fixes-css-1084 branch from f9b720a to 7c7ecde Compare January 7, 2025 20:14
@rise-erpelding rise-erpelding self-requested a review January 7, 2025 21:12
rise-erpelding
rise-erpelding previously approved these changes Jan 7, 2025
Copy link
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

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

Looks good to me! I know @marissahuysentruyt was planning to take another look at this, I'd be curious to know if her eagle eyes can spot anything here.

Documenting that I still think actionbutton is missing the basebutton styles and that's what's causing the two-colored outset border style, but we weren't able to get those base button styles to work on action button, so there may need to be a follow-up card for that (or we'll address it in the migration).
image

Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

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

I believe I found one or two more variables that should be defined here- let me know what you think!

components/actionbutton/index.css Show resolved Hide resolved
components/actionbutton/index.css Show resolved Hide resolved
@cdransf cdransf force-pushed the cdransf/undefined-token-fixes-css-1084 branch from b9e355e to 6cc39ea Compare January 8, 2025 19:32
@castastrophe castastrophe force-pushed the cdransf/undefined-token-fixes-css-1084 branch from 6cc39ea to a42f004 Compare January 13, 2025 16:56
@cdransf cdransf force-pushed the cdransf/undefined-token-fixes-css-1084 branch 2 times, most recently from 1d51314 to 49bf9c4 Compare January 13, 2025 19:44
@cdransf cdransf force-pushed the cdransf/undefined-token-fixes-css-1084 branch from f6805da to bc72ba7 Compare January 15, 2025 01:36
@cdransf cdransf force-pushed the cdransf/undefined-token-fixes-css-1084 branch from 5840e93 to 19f6bba Compare January 15, 2025 16:10
@cdransf
Copy link
Member Author

cdransf commented Jan 15, 2025

Wanted to drop a quick comment since I know this is still in review!

Since actionbutton particularly has been causing problems, I dug into this a bit more to try to figure out what's going on with it. Comparing how it's changed from s2-foundations-redux, I noticed that the styles that previously applied with the import of commons/basebutton.css are now hardcoded into the component, like cursor: pointer;, user-select: none; etc.

We lost the border-style: solid; in commons/basebutton.css in this commit so it didn't make it in when the commons styles were pasted in in this later commit. That was the style we needed for the button outlines and importing basebutton in this branch wasn't doing us any good because that style isn't in basebutton anymore.

Anyway, a couple of takeaways from that knowledge:

  1. We should probably paste that border-style: solid;, probably below this section:
/* Remove the inheritance of text transform on button in Edge, Firefox, and IE. */
	text-transform: none;

	/* stylelint-disable-next-line property-no-vendor-prefix -- Correct the inability to style clickable types in iOS and Safari (normalize). */
	-webkit-appearance: button;
  1. If things are getting really hairy with fixes, it might help to refer back to the way the code looked on the redux branch and how it changed between there and here. We know we have to add in any missing themes custom properties and ideally that would be it, but hopefully we can uncover any issues outside of that by looking back. 🤞

I just reverted

Wanted to drop a quick comment since I know this is still in review!

Since actionbutton particularly has been causing problems, I dug into this a bit more to try to figure out what's going on with it. Comparing how it's changed from s2-foundations-redux, I noticed that the styles that previously applied with the import of commons/basebutton.css are now hardcoded into the component, like cursor: pointer;, user-select: none; etc.

We lost the border-style: solid; in commons/basebutton.css in this commit so it didn't make it in when the commons styles were pasted in in this later commit. That was the style we needed for the button outlines and importing basebutton in this branch wasn't doing us any good because that style isn't in basebutton anymore.

Anyway, a couple of takeaways from that knowledge:

  1. We should probably paste that border-style: solid;, probably below this section:
/* Remove the inheritance of text transform on button in Edge, Firefox, and IE. */
	text-transform: none;

	/* stylelint-disable-next-line property-no-vendor-prefix -- Correct the inability to style clickable types in iOS and Safari (normalize). */
	-webkit-appearance: button;
  1. If things are getting really hairy with fixes, it might help to refer back to the way the code looked on the redux branch and how it changed between there and here. We know we have to add in any missing themes custom properties and ideally that would be it, but hopefully we can uncover any issues outside of that by looking back. 🤞

Alrighty! I just added that missing rule. I had tried reintroducing the action button updates by stepping through the changes again, but things still weren't quite right.

Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

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

Also wanted to document: spectrum-two.css defines this custom property (--spectrum-actionbutton-content-color-selected: var(--spectrum-gray-50);) in the foundations branch. This is what I see in foundations actionbutton/index.css for &.is-selected:

Screenshot 2025-01-15 at 1 29 34 PM

But in the spectrum-two branch, instead of using that spectrum-actionbutton-content-color-selected prop, we just use gray-25:
Screenshot 2025-01-15 at 1 31 59 PM

Should we refactor the gray-25 out of the mod definition during action button's migration? Like do something more like:

--spectrum-actionbutton-content-color-selected: var(--spectrum-gray-25);

&.is-selected {
		...

		--mod-actionbutton-content-color-default: var(--mod-actionbutton-content-color-default-selected, var(--spectrum-actionbutton-content-color-selected));
		--mod-actionbutton-content-color-hover: var(--mod-actionbutton-content-color-hover-selected, var(--spectrum-actionbutton-content-color-selected));
		--mod-actionbutton-content-color-down: var(--mod-actionbutton-content-color-down-selected, var(--spectrum-actionbutton-content-color-selected));
		--mod-actionbutton-content-color-focus: var(--mod-actionbutton-content-color-focus-selected, var(--spectrum-actionbutton-content-color-selected));
}

This is a change that isn't related to our undefined properties, so it's definitely a separate PR.

@cdransf cdransf force-pushed the cdransf/undefined-token-fixes-css-1084 branch from 2a3b8c4 to 30ae168 Compare January 15, 2025 19:53
@cdransf cdransf added size-5 L ~30-42hrs; lots of effort or complexity, most of a sprint needed to complete. blocked See description and comments for what is blocking this issue and removed size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. labels Jan 15, 2025
Copy link
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

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

Noting a couple of diffs I saw between this branch and s2-foundations that are causing some regressions!

components/actionbutton/index.css Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's this selector in s2 foundations that sets the border color to transparent that looks like its missing here, after --spectrum-logical-rotation. You have this selector here but I think would maybe need to add the border-color?

&.spectrum-ActionButton--quiet {
		--spectrum-actionbutton-border-color: transparent;
	}

Except that I don't think that works because now we're not setting --spectrum-actionbutton-border-color anywhere, so maybe try something like

                 --mod-actionbutton-border-color-default: transparent;
		--mod-actionbutton-border-color-hover: transparent;
		--mod-actionbutton-border-color-down: transparent;
		--mod-actionbutton-border-color-focus: transparent;
		--mod-actionbutton-border-color-disabled: transparent;

We're also missing setting the static-black-focus-indicator-color:

--mod-actionbutton-focus-indicator-color: var(--spectrum-static-black-focus-indicator-color);

And static white:

--mod-actionbutton-focus-indicator-color: var(--spectrum-static-white-focus-indicator-color);

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the spectrum-action-border-color change(s) should be handled in this commit: fef056. I stepped through the states on this branch/PR and matched them to a recent deploy with the previous, working state of the button(s).

image
image

@cdransf cdransf force-pushed the cdransf/undefined-token-fixes-css-1084 branch from 1c4b4a6 to fef056c Compare January 15, 2025 22:18
@rise-erpelding
Copy link
Collaborator

Hey! I think we've come a long way on action button and while we're still getting unintended borders on several quiet variants, I think a lot of this is due to some of the structural differences between the branches, which isn't really what we intended to fix here, and is proving to be a lot more difficult than originally anticipated. Overall, it looks fine and there are no glaring contrast or background issues that I'm seeing. If there are no other improvements that you or @marissahuysentruyt see to make on action button, I think that one's good to go.

When scanning the rest of the components fixed here, I did still notice some possible inconsistencies with button that I'd love some opinions on:
image
In this image, I think the second set of accent/negative buttons is supposed to be the outline variant? But also, I think button has already been migrated to S2 in this branch and it looks as if maybe those variants don't exist anymore. Could you confirm? It seems reasonable to think that we might need to remove test cases or maybe make some adjustments to the template after all the updates to spectrum-two, and if that's the case, it'd be ok to make another card for that.

As far as I can tell I think the other components look good!

Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

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

Running yarn linter against each component doesn't return any errors 🥳 With the project running, I don't see anything that's absolutely broken (wrong styles - yes, but nothing totally broken or unusable). I was a little concerned at the number of mods that look like they've been deleted in action button, but you mentioned in standup that they seemed self-referential, so maybe it's ok for the spectrum-two branch.

Like @rise-erpelding said, there's lots of work that probably could be done with action button. We'll have to readdress those things when the action button migration gets restarted. I also see the missing outlines in the test cases for outline buttons (she left that screenshot).

Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

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

Thanks for all your hard work on this!

Copy link
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

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

I think this is good to go!

Re-summarizing my notes:

  • Action button has several instances of visible borders where they should be transparent, but this doesn't seem to be because of missing custom properties so hopefully this can be taken care of in the migration.
  • Button - some follow up should be done to remove unneeded outline variants from tests since it's already been migrated and those variants don't exist anymore.

Clear button, Avatar, Dialog, and Infield button look good! Thanks @cdransf!

@cdransf cdransf merged commit a74476c into spectrum-two Jan 16, 2025
12 checks passed
@cdransf cdransf deleted the cdransf/undefined-token-fixes-css-1084 branch January 16, 2025 22:49
@cdransf cdransf removed the blocked See description and comments for what is blocking this issue label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review S2 Spectrum 2 size-5 L ~30-42hrs; lots of effort or complexity, most of a sprint needed to complete. skip_vrt Add to a PR to skip running VRT (but still pass the action)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants