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.

Sign up for GitHub

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

Jump to bottom

feat(slider): add outline-none styling for slider #102

Merged
merged 3 commits into from
May 24, 2023
Merged

feat(slider): add outline-none styling for slider #102

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bbdb84b
feat(slider): add outline-none styling for slider
CarolineGard May 17, 2023
65f58a7
fix(slider): add comment
CarolineGard May 23, 2023
1a76683
fix(slider): add comment
CarolineGard May 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/_rules/slider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,13 @@ export const sliderHandleShadow = [
}),
],
];

export const sliderOutlineNone = [
[
/^slider-handle-outline-none$/,
() => ({
outline: 'rgba(0, 0, 0, 0) solid 2px',
'outline-offset': '2px',
Comment on lines +24 to +25
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

}),
],
];
10 changes: 10 additions & 0 deletions test/__snapshots__/slider.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,13 @@ exports[`slider > get hover slider shadow 1`] = `
"/* layer: default */
.slider-handle-shadow-hover{box-shadow:0 0 0 6px rgba(0, 0, 0, .08);}"
`;

exports[`slider > get hover slider shadow 2`] = `
"/* layer: default */
.slider-handle-outline-none{outline:rgba(0, 0, 0, 0) solid 2px;outline-offset:2px;}"
`;

exports[`slider > get slider outline none 1`] = `
"/* layer: default */
.slider-handle-outline-none{outline:rgba(0, 0, 0, 0) solid 2px;outline-offset:2px;}"
`;
5 changes: 5 additions & 0 deletions test/slider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,9 @@ describe('slider', () => {
const { css } = await uno.generate(['slider-handle-shadow-hover']);
expect(css).toMatchSnapshot();
});

test('get slider outline none', async ({ uno }) => {
const { css } = await uno.generate(['slider-handle-outline-none']);
expect(css).toMatchSnapshot();
});
});