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

chore: merge branch 'main' into spectrum-two #3633

Merged
merged 19 commits into from
Mar 20, 2025

Conversation

jawinn
Copy link
Collaborator

@jawinn jawinn commented Mar 19, 2025

Description

Merges the latest main into spectrum-two. Helps with getting the tj-actions patch into spectrum-two.

Note: this ignored changes to the action button CSS during merge conflict handling, leaving it as-is on spectrum-two, as the full S2 migration is close to merging which replaces all this.

After merge, some stylelint warnings were thrown and two additional commits were added to fix an undefined variable in combobox, and a series of duplicated values that got into slider at a previous point.

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

  • PR builds without stopping during Verify
  • Nothing looks off in the changes and storybook runs normally.
  • In changed changelogs, the latest 2 changelog entries are in the right order (required some manual merge conflict handling).
  • No stylelint warnings posted on the PR. Look over last commit with fixes for combobox and slider.

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

To-do list

jawinn and others added 18 commits March 11, 2025 16:18
* feat(actionbutton): use closer to s2 colors in spectrum-two theme

Requested colors update for action button, aligning the colors closer
to the S2 design, post-foundations.

In the foundations spectrum-two theme:
- Removes the border
- Changes the default background colors to match s2 specs
- Updates the background colors used for static black and static white

SWC-497

* fix(actionbutton): fix high contrast styles for selected disabled

The selected + disabled button was not showing up as the disabled colors
in high contrast mode. Fixed by adjusting the source order slightly
in the high contrast media query so disabled is after selected and takes
precedence.
Co-authored-by: rise-erpelding <[email protected]>
Co-authored-by: [ Cassondra ] <[email protected]>
Bumps the npm_and_yarn group with 2 updates: [@babel/helpers](https://github.com/babel/babel/tree/HEAD/packages/babel-helpers) and [@babel/runtime](https://github.com/babel/babel/tree/HEAD/packages/babel-runtime).


Updates `@babel/helpers` from 7.26.0 to 7.26.10
- [Release notes](https://github.com/babel/babel/releases)
- [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md)
- [Commits](https://github.com/babel/babel/commits/v7.26.10/packages/babel-helpers)

Updates `@babel/runtime` from 7.24.4 to 7.26.10
- [Release notes](https://github.com/babel/babel/releases)
- [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md)
- [Commits](https://github.com/babel/babel/commits/v7.26.10/packages/babel-runtime)

---
updated-dependencies:
- dependency-name: "@babel/helpers"
  dependency-type: indirect
  dependency-group: npm_and_yarn
- dependency-name: "@babel/runtime"
  dependency-type: indirect
  dependency-group: npm_and_yarn
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
#3621)

* fix(stepper): focus/focus+hover/keyboardFocus border colors

* chore(stepper): add changeset
* feat(slider): offset variant border radius bug fix

* feat(slider): fix range slider
* fix(combobox): correct s1/legacy container variable

* fix(combobox): fast follow border color remapping
- add mods for s2 foundations disabled picker button BG/border colors
- correct disabled+read-only border color
- add read-only border custom property
- fixes express read-only border from gray-500 to gray-400
- update metadata.json

* chore(combobox): create changeset
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* fix(alertbanner): change system: spectrum to legacy
* chore(alertbanner): create changeset
* chore(checkbox): add isHovered state to checkbox

- adds isHovered shared type and control to checkbox stories
- adds several isHovered test cases
- updates checkbox template to include isHovered arg

* chore(form): fix the fieldgroup component input and labels
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
- add missing ::before pseudo to target the before element in the
invalid/checked/hover state
- update metadata.json
- create changeset
* chore: release

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link

changeset-bot bot commented Mar 19, 2025

⚠️ No Changeset found

Latest commit: 966487d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link
Contributor

github-actions bot commented Mar 19, 2025

File metrics

Summary

Total size: 1.37 MB*
No change in file sizes

Package Size Minified Gzipped
checkbox 22.69 KB 21.61 KB 2.64 KB
combobox 29.16 KB 28.13 KB 2.94 KB
slider 29.63 KB 27.97 KB 3.68 KB

File change details

checkbox

Filename Head Minified Gzipped Compared to base
index.css 22.69 KB 21.61 KB 2.64 KB 🔴 ⬆ 0.01 KB
metadata.json 12.75 KB - - 🔴 ⬆ 0.01 KB

combobox

Filename Head Minified Gzipped Compared to base
index.css 29.16 KB 28.13 KB 2.94 KB 🔴 ⬆ 0.33 KB
metadata.json 15.87 KB - - 🔴 ⬆ 0.16 KB

slider

Filename Head Minified Gzipped Compared to base
index.css 29.63 KB 27.97 KB 3.68 KB 🔴 ⬆ 0.12 KB
metadata.json 13.11 KB - - 🟢 ⬇ 0.08 KB
* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

Copy link
Contributor

github-actions bot commented Mar 19, 2025

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

@jawinn jawinn marked this pull request as draft March 19, 2025 19:52
@adobe adobe deleted a comment from github-actions bot Mar 19, 2025
castastrophe
castastrophe previously approved these changes Mar 19, 2025
Copy link
Collaborator

@castastrophe castastrophe left a comment

Choose a reason for hiding this comment

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

Updates are all looking great! Thanks for cleaning up some of the changelog diffs too. Cheers!

@jawinn Looks like some of the metadata.json assets need a refresh - can you re-run that before you merge?

@jawinn jawinn force-pushed the jawinn/spectrum-two-main-3-19-2025 branch 2 times, most recently from 8383c9e to a04fed7 Compare March 19, 2025 20:25
@jawinn jawinn marked this pull request as ready for review March 19, 2025 20:29
@jawinn jawinn marked this pull request as draft March 19, 2025 20:36
@jawinn jawinn marked this pull request as ready for review March 19, 2025 20:37
@jawinn jawinn force-pushed the jawinn/spectrum-two-main-3-19-2025 branch from a04fed7 to 658b7f7 Compare March 19, 2025 20:57
fix(slider): remove duplicated values

Remove a large number of duplicate values causing stylelint
"Unexpected duplicate" warnings. It looks like things got doubled up
somehow in a previous rebase or merge. This included duplicate t-shirt
size classes.

Also moves root styles block under the custom property definitions to be
consistent with other components.

fix(combobox): fixes undefined and duplicated values
@jawinn jawinn force-pushed the jawinn/spectrum-two-main-3-19-2025 branch from 658b7f7 to 966487d Compare March 19, 2025 21:12
@jawinn jawinn requested a review from castastrophe March 19, 2025 21:21
@jawinn
Copy link
Collaborator Author

jawinn commented Mar 19, 2025

@castastrophe can you take another look at this? It was converted to draft and in progress while you were approving. There were some stylelint issues with undefined and duplicated variables. I've added additional validation instructions.

@@ -56,6 +48,7 @@
--spectrum-combobox-border-color-focus: var(--spectrum-gray-800);
--spectrum-combobox-border-color-focus-hover: var(--spectrum-gray-900);
--spectrum-combobox-border-color-key-focus: var(--spectrum-gray-800);
--spectrum-combobox-border-color-disabled: var(--spectrum-disabled-border-color);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was undefined post-merge and needed to be brought over from the spectrum-two theme.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this wasn't coming from main - I'd rather we not try to add it in this PR now. We know there are lint issues on spectrum-two but that's not really a problem until we're ready to release.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe it was defined, but we had only been using it for the read-only combobox. I repurposed it for that most recent combobox release, when we needed to use it for all 3 themes (since the disabled border color changes).

Comment on lines -15 to -48
--spectrum-slider-track-color: var(--spectrum-gray-200);
--spectrum-slider-track-fill-color: var(--spectrum-gray-700);
--spectrum-slider-ramp-track-color: var(--spectrum-gray-400);
--spectrum-slider-ramp-track-color-disabled: var(--spectrum-gray-100);

--spectrum-slider-handle-background-color: transparent;
--spectrum-slider-handle-background-color-disabled: transparent;
--spectrum-slider-ramp-handle-background-color: var(--spectrum-gray-75);
--spectrum-slider-ticks-handle-background-color: var(--spectrum-gray-75);
--spectrum-slider-handle-border-color: var(--spectrum-gray-700);
--spectrum-slider-handle-disabled-background-color: var(--spectrum-gray-75);

--spectrum-slider-tick-mark-color: var(--spectrum-gray-200);

--spectrum-slider-handle-border-color-hover: var(--spectrum-gray-800);
--spectrum-slider-handle-border-color-down: var(--spectrum-gray-800);
--spectrum-slider-handle-border-color-key-focus: var(--spectrum-gray-800);
--spectrum-slider-handle-focus-ring-color-key-focus: var(--spectrum-focus-indicator-color);

--spectrum-slider-track-corner-radius: 2px;

--spectrum-slider-handle-border-radius: var(--spectrum-corner-radius-500);

&.spectrum-Slider--sizeS {
--spectrum-slider-handle-border-radius: var(--spectrum-corner-radius-500);
}

&.spectrum-Slider--sizeL {
--spectrum-slider-handle-border-radius: calc(var(--spectrum-corner-radius-500) * 4);
}

&.spectrum-Slider--sizeXL {
--spectrum-slider-handle-border-radius: calc(var(--spectrum-corner-radius-500) * 4);
}
Copy link
Collaborator Author

@jawinn jawinn Mar 19, 2025

Choose a reason for hiding this comment

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

These are all duplicated variables being flagged by stylelint on the PR. The sizing is already defined below this.

Comment on lines -114 to -130
position: relative;

/* Don't let z-index'd child elements float above other things on the page */
z-index: 0;
display: block;
min-inline-size: var(--mod-slider-min-size, var(--spectrum-slider-min-size));

user-select: none;

&:dir(rtl),
&:dir(rtl) {
--spectrum-logical-rotation: matrix(-1, 0, 0, 1, 0, 0);
}

&:not(.spectrum-Slider--sideLabel) .spectrum-Slider-labelContainer + .spectrum-Slider-controls:has(.spectrum-Slider-ramp) {
margin-block-start: calc(var(--mod-slider-ramp-track-height, var(--spectrum-slider-ramp-track-height)) / 2);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This block of styles was just moved down (outside of the variables).

@jawinn jawinn dismissed castastrophe’s stale review March 19, 2025 21:31

(was being worked on in draft when approved)

@castastrophe
Copy link
Collaborator

@castastrophe can you take another look at this? It was converted to draft and in progress while you were approving. There were some stylelint issues with undefined and duplicated variables. I've added additional validation instructions.

Yeah I wasn't too worried about the stylelint errors. The entire branch is a WIP so I didn't expect the linting to pass. The important part was the builds were running again.

@castastrophe
Copy link
Collaborator

@jawinn This feels out of scope for validation: No stylelint warnings. Look over last two commits with fixes for combobox and slider.

Spectrum-two is a work-in-progress anyway and I know for sure there are lint errors on there pre-merge with main.

@jawinn
Copy link
Collaborator Author

jawinn commented Mar 19, 2025

@castastrophe Would you rather I pull that last commit out into a separate PR? Then we would be merging this one with failing checks?

@castastrophe
Copy link
Collaborator

@jawinn Was something other than linting failing? Our CI logic is set up such that linting is allowed to fail on a PR and it can still be merged.

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.

As far as I can tell, this looks good to go. The changes I see in the combobox and slider CSS files look right to me 👍

I'm happy to approve, but am holding off just based on the comments about possibly pulling out the linting fixes for now.

@@ -56,6 +48,7 @@
--spectrum-combobox-border-color-focus: var(--spectrum-gray-800);
--spectrum-combobox-border-color-focus-hover: var(--spectrum-gray-900);
--spectrum-combobox-border-color-key-focus: var(--spectrum-gray-800);
--spectrum-combobox-border-color-disabled: var(--spectrum-disabled-border-color);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe it was defined, but we had only been using it for the read-only combobox. I repurposed it for that most recent combobox release, when we needed to use it for all 3 themes (since the disabled border color changes).

@castastrophe
Copy link
Collaborator

I don't want you to have to do rework to pull out fixes, if they look good to @marissahuysentruyt (which is sounds like they do) and you, no need to block this or do them separately. This is all good to merge.

@castastrophe castastrophe merged commit 9d0ab17 into spectrum-two Mar 20, 2025
24 checks passed
@castastrophe castastrophe deleted the jawinn/spectrum-two-main-3-19-2025 branch March 20, 2025 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants