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(internals): add rules for border-top/right/bottom/left-color classes #78

Merged
merged 2 commits into from
Mar 28, 2023

Conversation

BalbinaK
Copy link
Contributor

We've had rules for border width and style for different sides (top/right/bottom/left) and now we also have rules for internal classes that target color of those borders.

@BalbinaK BalbinaK requested a review from a team March 28, 2023 07:24
@BalbinaK
Copy link
Contributor Author


export const internalRules = [
[/^i-bg-(.+)$/, ([, cssvar]) => ({ 'background-color': h.warpToken(cssvar) })],
[/^i-text-(.+)$/, ([, cssvar]) => ({ color: h.warpToken(cssvar) })],
[/^i-border-(.+)$/, ([, cssvar]) => ({ 'border-color': h.warpToken(cssvar) })],
[/^i-border-([rltb])-(.+)$/, ([_, direction, cssvar]) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

No big thing 😄 but we don't use underscore variables for non-needed matches in the other regex callbacks.

Copy link
Contributor

Choose a reason for hiding this comment

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

And another thing... I am not quite sure how this works, but will it be a problem that for example i-border-r-$color-alert-warning-border will be matched by both /^i-border-(.+)$ and /^i-border-([rltb])-(.+)$/ ? 🤔 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently it works, according to the test output... but I can't understand how. 🤷 🙈 😅 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed underscore in 20824bf

As for why the /^i-border-(.+)$/ rule didn't work for i-border-l-$color-alert-warning-border, it was because the second argument of the callback was then"l-$color-alert-warning-border", and that when passed to the warpToken handler didn't match str.match(/^\$\S/) check. As a result the CSS property was assigned undefined as value, which makes unocss not generate any CSS for that selector. Does that make sense? 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok I think I get it... it DOES match the first regex and tries to create the border-color rule, but since h.warpToken() does not return anything the whole rule gets disposed. It then also matches the next rule, which creates the correct rule using the variable. Did I get that correct? 😄 ...Reading your comment again, I see that was about exactly what you said. 😄 Thanks. 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I checked, the cssvar string needs to start with a $ to be matched with the regex (it's because we say so with ^ at the beginning of the group). See here:
Screenshot 2023-03-28 at 14 37 26

Copy link
Contributor

Choose a reason for hiding this comment

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

Just trying to understand, and I am NOT saying that you should do this (😅)... but the best way would have been to write the first regex as to match anything but[rltb]- so that it wouldn't even try to create the rules? Is that correct? 🤷 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I understand now!
I think the reason for this check to be in the warpToken handler was that we'd keep the logic for ensuring correct prefixing in one place. So if we were to change how we separate prefixes from actual tokens, we wouldn't need to do it wherever that token could be used.
There might also come a time when we rewrite how the color tokens are translated into CSS in our preset, so I'm not sure if it's worth rewriting too much at this point. But that topic is still up for discussion at some point in the future (related potential task)

Copy link
Contributor

Choose a reason for hiding this comment

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

No, nothing that needs to be changed. 🙂 This just made me understand more about how this whole thing is working. 😄 Thanks for taking your time explaining. 🙂 🙏

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.

Lgtm 👍 Let's 🚢

@BalbinaK BalbinaK merged commit 83eb2f5 into alpha Mar 28, 2023
@BalbinaK BalbinaK deleted the feat/add-rules-for-border-lrtb-color branch March 28, 2023 12:19
github-actions bot pushed a commit that referenced this pull request Mar 28, 2023
# [1.0.0-alpha.5](v1.0.0-alpha.4...v1.0.0-alpha.5) (2023-03-28)

### Features

* **internals:** add rules for border-top/right/bottom/left-color classes ([#78](#78)) ([83eb2f5](83eb2f5))
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.

3 participants