-
Notifications
You must be signed in to change notification settings - Fork 3
PT-3049 Align tailwind.css with index.css #95
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
Conversation
tjcouch-sil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! Please wait and merge this when the other PR gets merged please :) just so we don't accidentally get things out of sync somehow
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
tjcouch-sil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
|
follow up needed for paranext/paranext-core#1622 |
tjcouch-sil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just a couple things to consider.
Reviewable status: all files reviewed, 2 unresolved discussions
src/tailwind.css line 72 at r2 (raw file):
--destructive: 0 62.8% 30.6%; --destructive-foreground: 210 40% 98%; /* slate-50 */ --border: 217.2 32.6% 17.5%;
These lines need to be changed, right?
From the core PR:
--border: 215.3 19.3% 34.5%; /* slate-600 */
--input: 215.3 19.3% 34.5%; /* slate-600 */src/tailwind.css line 154 at r2 (raw file):
@layer base { * { @apply tw-border-border;
Since it seems there are other changes already needed, would now be a good time to change this line to match shadcn's src/styles/globals.css file?
@apply border-border outline-ring/50;https://ui.shadcn.com/docs/installation/manual#configure-styles
|
@tjcouch-sil this has been hanging around for long, but seems not to have an effect / be needed / important - is this only a theoretical inconsistency or does it really have an effect to do this or not? |
tjcouch-sil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that theming is in the application such that it essentially overrides all the changes here (except the outline-ring/50 thing I mentioned; that does have an effect to my understanding. It's just subtle), this is not as important as it once was to keep up-to-date. However, it is still important in the sense that the tailwind classes that are built from extension code are out-of-date. Hopefully, they all get overwritten properly when the extension is running in core. But if we ever add a way to run extension code in other contexts such as in storybook (which I believe is desired to be added soon), the colors will be inconsistent between the extension storybook and running in core.
I'd recommend making the suggested adjustments, then we can merge this and be done. It's not super impactful such that we need to make sure it gets merged into every extension we own right this minute, but it would be good to have merged so it is ready for extensions to receive when they next update from template.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Sebastian-ubs)
d029a89 to
8432ef4
Compare
tjcouch-sil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tjcouch-sil reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
src/tailwind.css line 72 at r2 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
These lines need to be changed, right?
From the core PR:
--border: 215.3 19.3% 34.5%; /* slate-600 */ --input: 215.3 19.3% 34.5%; /* slate-600 */
Updated this section from core index.css. Recently there was a large rework of the themes, so much has changed.
src/tailwind.css line 154 at r2 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Since it seems there are other changes already needed, would now be a good time to change this line to match shadcn's
src/styles/globals.cssfile?@apply border-border outline-ring/50;https://ui.shadcn.com/docs/installation/manual#configure-styles
Done
lyonsil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lyonsil reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
side PR for paranext/paranext-core#1456 to get color variable updates to all extensions, see https://discord.com/channels/1064938364597436416/1344329166786527232/1348672085588967484This goes along with paranext/paranext-core#1547
This change is