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(slider): add outline-none styling for slider #102

Merged
merged 3 commits into from
May 24, 2023

Conversation

CarolineGard
Copy link
Contributor

Styling specific for the slider.
The outline styling is needed to make the focus state behave according to the design (the current design adds shadow and border)

To make the focus styling to work as acording to design
Copy link
Contributor

@BalbinaK BalbinaK left a comment

Choose a reason for hiding this comment

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

Styling specific for the slider. The outline styling is needed to make the focus state behave according to the design (the current design adds shadow and border)

Which design are you referring to? Would be really helpful with some before-after screenshots if possible, just to make the review process a bit easier 🙏

Comment on lines +22 to +23
outline: 'rgba(0, 0, 0, 0) solid 2px',
'outline-offset': '2px',
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember that there was an outline-none class before that would apply the outline and outline-offset as you did here, but it was said to be deprecated. Is this a use-case where we'd consider bringing it back? @Skadefryd @adidick @AnnaRybkina?

Copy link
Contributor

Choose a reason for hiding this comment

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

We do have https://github.com/warp-ds/drive/blob/alpha/src/_rules/focus-ring.js#L13 that adds outline-none but it is totally different I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to add a very clear TODO here(if we are going with this solution) that this way is not a preferred way of adding styling.
@Skadefryd is also suggesting that we add outline-offset rule to drive to handle line 23 as it is done in TW

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay agree, but we can go with this solution for now and add a todo, as it's a copy from the fabric solution? And we can modify it later when outline-offset is in place?

Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice stuff :) One question: Isnt it very fast to add the missing support to uno though like we have done for everything else?. In fact I would just add support for outline as a full thing. Its not just about this one component and this one usecase :) Should be fairly fast to add https://tailwindcss.com/docs/outline-width + https://tailwindcss.com/docs/outline-color + https://tailwindcss.com/docs/outline-style + https://tailwindcss.com/docs/outline-offset and be done with it. When it comes to why outline-none was removed .... I have no idea why. All these things in tailwind that can be added usually also has built in mecanisms for also turning them off again (very handy for inheritance problems and such amongst other things), these classes for turning stuff back off should in my opinion always be implemented aswell when we add something. Examples of that would be stuff like : -none, -inherit

Copy link
Contributor

@AnnaRybkina AnnaRybkina left a comment

Choose a reason for hiding this comment

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

🙈

Copy link
Contributor

@AnnaRybkina AnnaRybkina left a comment

Choose a reason for hiding this comment

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

🙈

@CarolineGard CarolineGard merged commit 48fdd8a into alpha May 24, 2023
3 checks passed
@CarolineGard CarolineGard deleted the add-outline-none-to-slider branch May 24, 2023 12:50
github-actions bot pushed a commit that referenced this pull request May 24, 2023
# [1.0.0-alpha.27](v1.0.0-alpha.26...v1.0.0-alpha.27) (2023-05-24)

### Features

* **slider:** add outline-none styling for slider ([#102](#102)) ([48fdd8a](48fdd8a))
github-actions bot pushed a commit that referenced this pull request Aug 22, 2023
# 1.0.0 (2023-08-22)

### Bug Fixes

* add 75% to the opacity values ([#86](#86)) ([fa52e6e](fa52e6e))
* add arbitrary width support for border ([#88](#88)) ([2eb8d87](2eb8d87))
* add base styles to preflight ([#101](#101)) ([19ec46b](19ec46b))
* add bg color options  ([#76](#76)) ([980c532](980c532))
* add correct token for the focus ring ([#87](#87)) ([9191bca](9191bca))
* add development flag back and externalize our published component classes ([#129](#129)) ([4470892](4470892))
* add font-bold to t1-t6 and use those classes to ref h1-h5 ([#146](#146)) ([1c6a81d](1c6a81d))
* add missing apostrofs ([#91](#91)) ([a28756e](a28756e))
* add missing classes Fabric supported ([#135](#135)) ([4ce6033](4ce6033))
* add px to the arbitrary border width ([#93](#93)) ([c4d17ae](c4d17ae))
* add rules and tests for caret ([#84](#84)) ([fac54a1](fac54a1))
* add skipResets flag ([#142](#142)) ([a13cbcd](a13cbcd))
* add styling for loading ([#77](#77)) ([87e0e50](87e0e50))
* Added missing rules for static border colors  ([#123](#123)) ([6644a66](6644a66))
* **aspect-ratio:** add rules for aspect-ratio, -video and -square ([#150](#150)) ([f5d1c79](f5d1c79))
* avoid null issue and use empty array instead ([#144](#144)) ([6d56dcd](6d56dcd))
* avoid top-level await in reset ([#137](#137)) ([b35c803](b35c803))
* background arbitrary url rule change ([#95](#95)) ([ae70e51](ae70e51))
* bad comment syntax in preflight ([#73](#73)) ([ae37e8e](ae37e8e))
* be able to use a one-off background image ([#89](#89)) ([12df5a9](12df5a9))
* border-x|y fix and slider obsolete snapshot fix ([#120](#120)) ([85dfaec](85dfaec))
* cache the preflight instead of requesting every rebuild ([83537ca](83537ca))
* Cleanup color classes for text and background color ([#127](#127)) ([1b3d13e](1b3d13e))
* **display.js:** align order of css for display classes with fabric.css ([#147](#147)) ([aa00cfc](aa00cfc))
* escape current selector for focusable ([#122](#122)) ([00b789b](00b789b))
* extend regex for the background arbitrary values ([#92](#92)) ([d908e24](d908e24))
* focusable classes ([#109](#109)) ([8e3ca96](8e3ca96))
* focusable, last-child, spinner rules ([#110](#110)) ([40e75cf](40e75cf))
* height and width shouldn't match without a dash ([#83](#83)) ([3580750](3580750))
* improve arbitrary bg rule ([#111](#111)) ([ea43100](ea43100))
* improve syntax to clear the float for the elements after legend ([#108](#108)) ([75f4cac](75f4cac))
* only support gap from theme ([#31](#31)) ([4d49678](4d49678))
* proper handle bg arbitrary url as var ([#126](#126)) ([0ddd0e9](0ddd0e9))
* remove @warp-ds/css dep to fix circular dependency ([#149](#149)) ([e351c76](e351c76))
* remove a shortcut for the outline-none ([#128](#128)) ([ced819c](ced819c))
* remove caret class, will use current ([#85](#85)) ([f87db11](f87db11))
* remove input related stuff from preflight ([#81](#81)) ([a5dcb42](a5dcb42))
* Remove old fallback values from box shadows ([#140](#140)) ([da00995](da00995))
* remove resets from preflight and refactor ([#106](#106)) ([3591a3b](3591a3b))
* Replace arbitrary background class with semantic class for page-container ([#138](#138)) ([ee9c296](ee9c296))
* restructure html and body styling to make shadow dom styling work ([#107](#107)) ([7eb3e94](7eb3e94))
* **shadows:** add default box-shadow values to generic shadow classes ([#96](#96)) ([4adb2d6](4adb2d6))
* **shadow:** update naming of box-shadows ([#112](#112)) ([4405ae5](4405ae5))
* **slider:** fix slider bug ([#100](#100)) ([31b9b0c](31b9b0c))
* theme now exports useTheme ([#82](#82)) ([88476fd](88476fd))
* throw better errors for users who are missing 'fetch' ([#132](#132)) ([77ba1e2](77ba1e2))
* trigger a new release of alpha.59 ([#145](#145)) ([1ad8465](1ad8465))
* **typography.js:** add font-bold to .h1-5 shortcuts ([#121](#121)) ([b84a631](b84a631))
* update & align unocss dependencies ([#148](#148)) ([e540d19](e540d19))
* Update focus outline to use new semantic color token ([#139](#139)) ([673627d](673627d))
* update vite to 4.2.3 to fix a vulnerability ([#152](#152)) ([929c9af](929c9af))
* Updated rules for semantic colors to handle base level token names without ending "default" suffix ([#133](#133)) ([805526d](805526d))
* use #plugin alias in import in checkFabricClasses ([6022115](6022115))
* use new css repo for component classes ([#143](#143)) ([b2816b6](b2816b6))
* **workflows:** set node v to lts/* and pnpm to 8 ([#118](#118)) ([413058d](413058d))
* Wrong syntax ([#69](#69)) ([b938d3a](b938d3a))

### Features

* Add background, text and border classes for semantic color tokens ([#119](#119)) ([19b5ee9](19b5ee9))
* add css variables as arbitrary values for w and h ([#104](#104)) ([455c4cb](455c4cb))
* add layout rules and tests for rules ([d47d412](d47d412))
* add new shortcut for page-container ([#114](#114)) ([8ae08b3](8ae08b3))
* add rules for border and rounded ([#19](#19)) ([48ec500](48ec500))
* add rules for focusable classes ([#27](#27)) ([e9f7e08](e9f7e08))
* add rules for outline ([#125](#125)) ([3834087](3834087))
* add rules for shadows ([#90](#90)) ([a0dfbc5](a0dfbc5))
* add rules for table ([#33](#33)) ([d1ea3fb](d1ea3fb))
* add rules for text decoration ([#29](#29)) ([9797311](9797311))
* add tests for align rules ([64d2815](64d2815))
* arbitrary values accepted for leading ([#130](#130)) ([39919f4](39919f4))
* automate package publishing using semantic-release  ([#68](#68)) ([9273e19](9273e19))
* **dropshadow:** add drop shadow ([#105](#105)) ([9b68cbe](9b68cbe))
* **internals:** add rules for border-top/right/bottom/left-color classes ([#78](#78)) ([83eb2f5](83eb2f5))
* **internals:** add rules for shadow component classes ([#94](#94)) ([e7e65d1](e7e65d1))
* rule for adding safe area inset from the bottom edge of the viewport ([#103](#103)) ([88186f6](88186f6))
* **rules:** add typography rules ([#99](#99)) ([6e1c363](6e1c363))
* **semantic.js:** add rules for semantic icon colors ([#124](#124)) ([afc0354](afc0354))
* **slider:** add border shadow for active and hover states ([#98](#98)) ([0b36581](0b36581))
* **slider:** add outline-none styling for slider ([#102](#102)) ([48fdd8a](48fdd8a))
* **touch-actions:** add support for touch actions ([#97](#97)) ([54a3002](54a3002))
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.

None yet

4 participants