Style-sheet-vars #490
Style-sheet-vars #490
Conversation
|
Sorry, I changed some styles out from under you. Just a quick note to make sure we don't the changes in b1214a9, because they let us do this:
|
src/module/helpers/handlebars.ts
Outdated
| } clickable svg ironsworn__progress__rank" | ||
| }s svg ironsworn__progress__rank" |
| @@ -71,6 +71,7 @@ | |||
|
|
|||
| <style lang="less"> | |||
| .progress-completed { | |||
| margin-top: 1rem; | |||
There was a problem hiding this comment.
This is getting clobbered by .item-row
| } | ||
| } | ||
| </style> | ||
|
|
||
| <style lang="less" scoped> | ||
| .progress-completed { | ||
| } |
There was a problem hiding this comment.
Need this or no? We've also got a scoped style sheet and a global one here. Do we need both, or should we standardize on one of them?
| </div> | ||
| </div> | ||
| </template> | ||
|
|
||
| <style lang="less" scoped> | ||
| .stat { | ||
| cursor: pointer; |
There was a problem hiding this comment.
This specific change in this PR sets the pointer style, but in edit mode it feels weird to have a hand pointer when clicking doesn't actually do anything (only the "+" and "-" are clickable). Also (but not because of this PR) we show a tooltip that's wrong.
There was a problem hiding this comment.
Oh, also: looks like we have warring transitions going on in Chrome.
CleanShot.2022-10-16.at.06.54.20.mp4
| min-height: inherit; | ||
| } | ||
| .editor-edit { | ||
| background-color: var(--ironsworn-color-btn-bg-enabled); |
There was a problem hiding this comment.
Looks like this is turning the background gray. Is this intentional? This seems to go counter to the Foundry default and the norm of the other systems I tried. I'm not sure I like it.
dnd5e in Foundry v9:
Blades in the dark, which is so minimal I assume it's the default Foundry styles:
And 13th Age, which actually gets lighter in edit mode:
There was a problem hiding this comment.
the idea is to provide a visual cue for "this is a (rich) text input box" that has some relationship to other (plain) text input boxes, at least when the field is editable.
ultimately, tho, harmonizing that might require more aggressive restyling of the editor component than is practical to include in this PR (and are inessential to its purpose, anyways)
| @@ -10,8 +10,6 @@ const props = defineProps<{ value: number; current: number }>() | |||
|
|
|||
| const classes = computed(() => { | |||
| return { | |||
| clickable: true, | |||
| block: true, | |||
There was a problem hiding this comment.
These lost their hover state:
CleanShot.2022-10-16.at.07.17.50.mp4
There was a problem hiding this comment.
yeah, my plan was to restore that before removing the draft status. the method to my madness here was roughly:
- draw up a palette of colour variables, organized by attributes of the colour ("light", "dark", etc)
- migrate all styles specific to a single component to the corresponding SFC
- standardize component css variables
- root out anything that looks like a good CSS variable candidate and replace it with one
- synthesize formal standards/patterns from existing de facto standards/patterns where noticed, taking care to maintain their coherence when considered as a group
- develop new variables as needed, and organize by purpose or semantics of the variable (referencing the palette vars, above)
- periodically stop to re-evaluate their semantics/organization
- (partially complete) pick over the rendered sheet, adjusting values until it renders at parity (though not necessarily identically) with the original
- review and prune CSS variables, removing ones that are underutilized, etc
- review PR's diffs, culling changes that are trivial or deferrable, plus any placeholder code, fixable FIXMEs, or weird artifacts
- untick PR's draft status
| @@ -0,0 +1,65 @@ | |||
| .ironsworn { | |||
There was a problem hiding this comment.
Is this file meant to be deleted when you're sure you don't need it anymore?
There was a problem hiding this comment.
yeah, that's the idea. this PR is pretty firmly in the Draft Zone still, and i was mostly looking for feedback on its general direction before i sank more time into it.
| @import './ironsworn-icons.less'; | ||
| @import 'mixins.less'; | ||
| @import 'mixin-utils.less'; | ||
| @import 'attr-box.less'; | ||
| @import 'button.less'; | ||
| @import 'chat-messages.less'; | ||
| @import 'compact-sheet.less'; | ||
| @import 'dialogs.less'; | ||
| @import 'forms.less'; | ||
| @import 'icons.less'; | ||
| @import 'item-list.less'; | ||
| @import 'layout.less'; | ||
| @import 'rolls.less'; | ||
| @import 'sidebar.less'; | ||
| @import 'tables.less'; | ||
| @import 'transitions.less'; | ||
| @import 'typography.less'; | ||
| @import 'widgets.less'; |
There was a problem hiding this comment.
So this reorg I think is emblematic in how our styles differ. 😁 This PR is an 83-file monster that's pretty hard to review, because there are at least 2 separate mostly-mechanical refactorings mixed in with at least 3 substantive changes to the styles themselves. On its own, I 😍 this specific change a lot. But it's super hard to review when a big reorg like this is mixed in with changes to the content that's being moved around – this file has 875 lines of diff, and GitHub didn't even display it by default.
One way to make this easier to manage would be to back out and do specific, targeted refactorings in their own PRs. One PR to split this CSS into multiple files (easy to review and merge), one PR to introduce and roll out a global border variable (easy to review and merge), and so on. Does that make sense?
heavily WIP. touches a lot of files, but most of the changes are just plugging in CSS variables to SFCs. not ready for a review, but the SF sheet (with the SF theme) is shaping up OK so far (on FF at least).