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

feat(color-picker): replaces thumb focus outline to rounded #7378

Merged
merged 27 commits into from
Aug 1, 2023

Conversation

anveshmekala
Copy link
Contributor

@anveshmekala anveshmekala commented Jul 25, 2023

Related Issue: #4633

Summary

This PR will replace the existing focus outline for thumb in calcite-color-picker to rounded.

After the change:
494C307B-315E-4842-93C5-911C6F3810A2
ED840D87-BFA5-4602-8D1C-51FF78CD04E2

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Jul 25, 2023
@anveshmekala anveshmekala marked this pull request as ready for review July 26, 2023 21:54
@anveshmekala anveshmekala requested a review from a team as a code owner July 26, 2023 21:54
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

👍 🎉 🌮

role="slider"
style={{ top: `${hueTop}px`, left: `${hueLeft}px` }}
style={{
top: `${hueTop - DIMENSIONS.scopeNode / 2}px`,
Copy link
Member

Choose a reason for hiding this comment

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

since a few places are dividing a value by the scopeNode and converting to px maybe convert that into a function for reuse.

@anveshmekala anveshmekala added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jul 26, 2023
@anveshmekala anveshmekala marked this pull request as draft July 26, 2023 22:23
@anveshmekala anveshmekala marked this pull request as ready for review July 27, 2023 16:52
@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jul 27, 2023
@anveshmekala anveshmekala requested a review from driskull July 27, 2023 19:17
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Looking good, @anveshmekala! ✨🖍️😎✨

@@ -93,4 +93,5 @@ export const DIMENSIONS = {
radius: 10,
},
},
thumbNode: 1,
Copy link
Member

Choose a reason for hiding this comment

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

Can this be moved to the component itself or a separate constant? Currently, I find it a bit confusing:

  1. the dimensions object stores the size of different parts across all scales that are drawn onto canvas elements
  2. I know it's a bit verbose, but repeated values across scales have a corresponding prop
  3. none of the existing part names use node, and there's already one for thumb

&:focus {
@apply focus-outset;
outline-offset: 16px;
Copy link
Member

Choose a reason for hiding this comment

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

One more thing I noticed. The design spec seems to have a 2px space between the thumb and the focus outline. The spacing in the PR screenshot looks larger than that. Maybe adjusting this to 14px can help better match that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we increase the spacing from 12px to 14px it will increase the gap between the thumb and outline.

Thoughts @ashetland

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding @macandcheese.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I’m following, I agree it should be a 2px gap to align with other “outset focus” styles.

@anveshmekala anveshmekala marked this pull request as draft July 27, 2023 23:12
@anveshmekala anveshmekala marked this pull request as ready for review July 28, 2023 22:59
@anveshmekala anveshmekala requested a review from jcfranco July 28, 2023 22:59
@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jul 31, 2023
@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jul 31, 2023
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Aside from some minor comments, this LGTM! 🚀

@@ -22,6 +22,10 @@ describe("calcite-color-picker", () => {
await page.waitForChanges();
}

function getScopeCenter(X: number, Y: number): [number, number] {
Copy link
Member

Choose a reason for hiding this comment

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

✨🏆✨

});
});
});

describe("mouse interaction with scope", () => {
Copy link
Member

Choose a reason for hiding this comment

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

Awesome coverage here. Can you move this closer to the keyboard scope interaction block? Bonus points if these are grouped within a scope interaction describe block.

describe("scope interaction", () => {
  describe("mouse", async () => { /* ... */ });
  describe("keyboard", async () => { /* ... */ });
})

This is not critical and can be done in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently mouse interaction with scope is placed near to keyboard scope interaction block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will group them.

const [hueScopeX, hueScopeY] = await getElementXY(page, "calcite-color-picker", `.${CSS.hueScope}`);
const value = await colorPicker.getProperty("value");

await page.mouse.move(hueScopeX + 0.8, hueScopeY);
Copy link
Member

Choose a reason for hiding this comment

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

What does the 0.8 represent? If it's an arbitrary move in pixels, can you simplify this by using whole numbers? Also, can you assign it to a self-documenting const?

This applies to the 10 and 2 values below too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the 0.8 represent the pixel inside the scope element and the reason for using value above 0.5 is to avoid getting rounded off and assert a change in value.

@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jul 31, 2023
@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Aug 1, 2023
@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Aug 1, 2023
@anveshmekala anveshmekala merged commit d803980 into main Aug 1, 2023
@anveshmekala anveshmekala deleted the anveshmekala/4633-color-picker-fix-focus-outline branch August 1, 2023 19:56
@github-actions github-actions bot added this to the 2023 July Priorities milestone Aug 1, 2023
@geospatialem geospatialem mentioned this pull request Aug 1, 2023
7 tasks
benelan pushed a commit that referenced this pull request Aug 3, 2023
**Related Issue:** #4633 

## Summary
This PR will replace the existing focus outline for thumb in
`calcite-color-picker` to rounded.

After the change:

![494C307B-315E-4842-93C5-911C6F3810A2](https://github.com/Esri/calcite-design-system/assets/88453586/02b8daed-05c8-4d98-ad1b-504fe9d58fd3)

![ED840D87-BFA5-4602-8D1C-51FF78CD04E2](https://github.com/Esri/calcite-design-system/assets/88453586/4599dc2a-aad4-4a48-b6fd-57277a9b0884)
benelan pushed a commit that referenced this pull request Aug 3, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>@esri/calcite-components: 1.5.0</summary>

##
[1.5.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected])
(2023-08-03)


### Features

* **action-group:** Adds overlayPositioning property.
([#7366](#7366))
([ca9f35a](ca9f35a))
* Allow sharing focus trap stacks via configuration global
([#7334](#7334))
([934a19f](934a19f))
* Automatically import and define Calcite Components when importing
their React wrapper
([#7185](#7185))
([bf0ff67](bf0ff67))
* **block, block-section:** Add setFocus method
([#7208](#7208))
([35d4bbb](35d4bbb))
* **block:** Improve block's content layout to allow scrolling
([#7367](#7367))
([ecbf17b](ecbf17b))
* **color-picker:** Replaces thumb focus outline to rounded
([#7378](#7378))
([d803980](d803980))
* **filter:** Add filter method
([#7127](#7127))
([5a4283f](5a4283f))
* **flow:** Adds setFocus method
([#7252](#7252))
([2472c58](2472c58))
* Improve focus behavior in components
([#7277](#7277))
([ad9fbca](ad9fbca))
* **input-time-zone:** Add input-time-zone component
([#6947](#6947))
([87bd496](87bd496))
* **list:** Add slots for filter actions
([#7183](#7183))
([da07ab1](da07ab1))
* **list:** Add support for dragging items.
([#7109](#7109))
([7324f70](7324f70))
* **menu-item:** Update spacing and icon layout
([#7381](#7381))
([5659671](5659671))
* **navigation-logo:** Increase font-size of heading with no description
([#7081](#7081))
([355e101](355e101))
* **switch:** Updates focus outline to be rounded
([#7390](#7390))
([2616b82](2616b82))
* **text-area:** Provide additional context for AT users when character
limit exceeds
([#7299](#7299))
([c5678eb](c5678eb))
* **text-area:** Provide additional context for AT users when character
limit exceeds
([#7412](#7412))
([c1af3c7](c1af3c7))


### Bug Fixes

* **accordion, accordion-item:** `icon-position`, `icon-type`,
`selection-mode` and `scale` can now be set as props or attributes
([#7191](#7191))
([2b09aba](2b09aba))
* **action-bar:** No longer delegates focus when clicked on
non-focusable region
([#7310](#7310))
([1a9c15c](1a9c15c))
* **action:** Correctly focus the button after rendering updates.
([#7255](#7255))
([40fe2ce](40fe2ce))
* **block:** Loader now appears for all loading cases
([#7303](#7303))
([5af3600](5af3600))
* **block:** Removes extra loading indicator
([#7239](#7239))
([a334a75](a334a75))
* **card:** Ensure teardown logic is called when disconnected
([#7289](#7289))
([d07e322](d07e322))
* **chip:** Disconnect mutation observer when component is disconnected
from the DOM
([#7418](#7418))
([412e5fb](412e5fb))
* **color-picker:** Draw slider thumbs within bounds
([#7398](#7398))
([2f37854](2f37854))
* **color-picker:** Fix opacity slider keyboard nudging
([#7400](#7400))
([2b4f7c3](2b4f7c3))
* **color-picker:** Maintains correct numbering system when entering
invalid RGB value
([#7327](#7327))
([8d2a3a5](8d2a3a5))
* **combobox:** Add space after grouped items
([#7302](#7302))
([b1580c7](b1580c7))
* **dropdown-item:** Provides accessible label when href is not parsed
([#7316](#7316))
([966b83d](966b83d))
* **flow:** Call setFocus() on back button click
([#7285](#7285))
([9102aa4](9102aa4))
* **input-date-picker:** Provides placeholder text context for AT users
([#7320](#7320))
([31e0ba2](31e0ba2))
* **input-date-picker:** Reset active date picker date after closing
([#7219](#7219))
([91b2a1b](91b2a1b))
* **input, input-number:** No longer removes trailing decimal separator
([#7159](#7159))
([01535cf](01535cf))
* **link:** Adds outline-offset to avoid overlapping with text.
([#7342](#7342))
([c30db4e](c30db4e))
* **list:** Changing filterText property will now update filtered items
([#7133](#7133))
([a9c0bce](a9c0bce))
* **list:** Fix keyboard navigation after a listItem's disabled or
closed property changes
([#7275](#7275))
([91d28eb](91d28eb))
* **list:** Fix keyboard navigation when filterEnabled is true
([#7385](#7385))
([41a2e42](41a2e42))
* **menu-item:** Prevent duplicate border in nested vertical menu-items
([#7387](#7387))
([186a738](186a738))
* **panel:** Remove double border styling when content isn't provided
([#7368](#7368))
([91a0610](91a0610))
* Remove style modifying all host components with hidden attribute
([#7346](#7346))
([3103e2f](3103e2f))
* **scrim:** Update loader scale on resize of component.
([#7419](#7419))
([24e7f70](24e7f70))
* **slider:** Prevent excessive tick rendering
([#7421](#7421))
([c799409](c799409))
* **switch:** Fix for focus outline style in certain cases
([#7414](#7414))
([217324f](217324f))
* **tab-title:** Add full focus outline to closable tab button in high
contrast mode
([#7272](#7272))
([d812d17](d812d17))
* **tooltip:** Avoid extra before open/close event emitting
([#7422](#7422))
([dbb6818](dbb6818))
* **tooltip:** Deprecate the label property due to the description
coming from the component's content
([#7247](#7247))
([7934d75](7934d75))
* **tooltip:** Emits `close` and `beforeClose` events when container is
set to `display:none`
([#7258](#7258))
([60a4683](60a4683))
* **tooltip:** Ensure --calcite-app-z-index-tooltip is applied
([#7345](#7345))
([a9a7072](a9a7072))
</details>

<details><summary>@esri/calcite-components-react: 1.5.0</summary>

##
[1.5.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected])
(2023-08-03)


### Features

* Automatically import and define Calcite Components when importing
their React wrapper
([#7185](#7185))
([bf0ff67](bf0ff67))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from ^1.5.0-next.38 to ^1.5.0
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants