feature/move-styles-to-components #874
No reviewers
Labels
No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
No Milestone
No project
No Assignees
2 Participants
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#874
Loading…
Reference in New Issue
No description provided.
Delete Branch "dpschen/frontend:feature/move-styles-to-components"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Move styles to components
By moving the styles to the components that use them and scoping the styles there we keep our future styling options more open.
The scoping allows us to use a different approch in one component and incrementally adopt that everywhere.
Some styles will be broken simply because it was not possible for me to see where some styles apply since they were defined globally.
I tried my best to fix these cases, some still remain.
There are still some styles left that apply globally.
With these styles I was not able to figure out where they apply – which is bad. A style where you don't know where it affects things is a style that you are not able to maintain anymore. A few styles should stay global (e.g. scrollbar styles, some resets or font definitions). The rest should either be defined as mixins to be included in multiple locations or moved to the components.
It's best to see what happened with the styles by looking at the individual commits. I tried to make it as easy as possible to follow.
I would love to merge this as fast as possible because its hard to keep up with the main branch because of the pull requests size.
(some) open issues
color-picker
anddate-picker
need variables that are defined inside the bulma components scss files.Since we don't want to include scss files with a compiled output in the scss additionalData that gets prefixes before every components scss code we need to import the component where the variables get defined manually:
bulma/sass/components/card.sass
.Currently I have no other solution for this.I have an idea how to fix this, but I have to experiment a bit to verify that idea.See
7a95d08e1e
.https://vue-loader.vuejs.org/guide/scoped-css.html#deep-selectors=> https://v3.vuejs.org/api/sfc-style.html#deep-selectors). For an example look atcomponents/input/datepicker.vue
.TheThis was wrong. I didn't understand::v-deep
selector is deprecated and should be replaced by:deep(<inner-selector>)
. The problem with the latter is that it makes writing nested styles like we use very often impossible to write. The reason is that theinner-selector
can't contain multiple nested childs. This can be solved by either moving these styles in the components they apply. I didn't do that yet because most of the time this would lead to new variants of that component (with different styling).:deep()
completely. The selector seems to work.I think this is a good chance to format all css to use tabs instead of spaces consistently - could you do that?
@ -118,2 +118,4 @@
})
</script>
<style lang="scss">
Did you put this seperately from the other style tag to prevent importing and prefixing these global styles in the scope? (possibly multiple times)
Not sure if I 100% understand the question. But I'll just try to explain why there are two style tag in this component:
the first (without
scoped
attribute) contains all global styles. The are just loaded here for the whole application.Since they are global the CSS rules inside also apply to all other components. Our target is to minimize the content of this file.
But some will always stay there. E.g. CSS resets, typo imports.
The style with the
scoped
attribute contains the styles that get scopedby the vue loader(vue-loader was in vue 2) to the current compoent via the css data attributes. I guess we could also abstract the offline stuff in its own component then these styles would be there together with the template.When compiled
by the vue-loaderboth of these kind of styles will get thecompiled output of the scss defined in the
additionalData
in the vite.config.js which should only be variables and mixins (so it should not result in rendered CSS code). With this "trick" (I think its kind of the way to go for vue) every component has all the global variables available. In reality every component imports them again on their own.In general I tried to change as few as possible with the styles. But for example below we could just unindent all styles and decrease the complexitiy by decreasing the specifity:
Even if there is an
<h1>
somewhere else in the app: just the one in this component should be styles by this rule because of the scoping =)Almost every component in this pull request could be unindented in this way.
The
font-weight: 700 !important;
is a topic in itself :)Thanks for clarifying!
Doesn't that bloat all component stylesheets because they all contain all global stylesheets? Or is vite smart enough to figure that out and create small bundles?
We should clean the stylesheets up at some point. They've all grown over time ("historisch gewachsen") 🙂 I think that's better for another PR though because this one is already quite big.
@ -5,7 +5,6 @@
:disabled="disabled || null"
:id="checkBoxId"
@change="(event) => updateData(event.target.checked)"
style="display: none;"
Good catch.
@ -48,3 +1,1 @@
.edit-list {
padding-bottom: 1rem;
}
// FIXME: should be a component <FilterContainer>
I think we should track these - do you want to open an issue or should I?
@ -1,3 +1,8 @@
// FIXME:
// this should be moved to button.vue
// what prevents this currently is that the class is also used
// on a <select> element in userTeam.vue
I think the class in
userTeam.vue
is using the bulma styling though so it should be save to remove this.That's cool! =)
So you mean I can just remove the
button
class in the userTeam.vue and then there should be no problem in moving this to the button.vue?Done
Are these variables defined with
!default
? Then we could just define them in "our" scss variables config file instead, couldn't we?There's a few things I noticed:
Scrolling on the kanban board view - there's a pretty hacky height calculation I did there to prevent that (look for uses of
$crazy-height-calculation
)The list menu on the top title bar needs some spacing:
I think we should fix at least some of those before merging this.
Maybe using a tool like percy could a good idea here?
About the tooltips: Using something like https://cooltipz.jackdomleo.dev/ might be a good idea. In the past I've used a vue-tooltip library but that was really bloated so I tried to replace it with my own attempt. That works kinda okay but has some edge-casey bugs where the tooltips don't disappear in some conditions. And they are not reactive.
(not in this PR though)
I have some good experience with popper.js. It's basicalally a pure battle tested tooltip positioning libary.
Building something from scratch is really hard as you can tell. And pure CSS solutions have their flaws.
I would not use Tippy.js which is their own attempt to build tooltips on top of popper.js. I think it's kind of bloated. Instead it's relly easy to build a simple implmentation on your own. Maybe I have somewhere still a component. Will check :)
Do you have some recommendations how to automate that?
I would love to use prettier for that but that affects everything and not just the CSS.
Sadly that doesn't (currently) work.
The reason is that the in these variables are just defined in
bulma/sass/components/card.sass
and we cannot include all the component styles in the scss of theadditionalData
because it generates rendered CSS that would then be included and repeated in every component. Gzip helps a lot. But that would would basically multiply the size of bulma by the amount of components we have =)Because we normally use just the base variables that are defined in bulmas utilities and bulmas shared form styles and both of these don't create a rendered css output we can include them in the
additionalData
.So by default we can use all the variables of bulma that are not defined in the components. Not to be confused with the already rendered classes that used our overwritten variables in the global bulma import.
I hope that was understandable.
EDIT: Could solve the issue by using
$shadow
directly which is the variable that defined$card-shadow
. See:7a95d08e1e
.For the future for sure. But right now I guess it's more effort to implement that than to just check if everything looks good or not?
Regarding the things you noticed: all valid stuff!
I think some of this happened when I rebased :/ Some of this was already working before :P
I'll go through each one by one and compare with the old version.
Tracking those fixes here:
=> Done for this case. There might be more issues of this kind.
I forgot to add a space between the
::v-deep
and the next selector (in this case.dropdown-trigger
) which means that the next selector will be rendered as part of the current one. This is similar to the use of&.selector
vs& .selector
in pure SCSS.$crazy-height-calculation
)Note: I write CSS for ages. And I didn't know that you can escape characters from a class (the
.app-content.list\.kanban
). Learned something new 🎉@konrad: all the issues that you listed are solved. Anything else blocking this?
The second open issue from the pull request description is something that I think makes sense to solve over time.
I've used v-tooltip without thinking much about it but found 70kb (20 gzipped) too much for a tooltip library. That's why I removed it and created my own implementation of it. I didn't actually think about popper.js, it looks like that should work really well though. Only used it in non-vue contexts so far. IMHO tooltips should be very easy to use, a directive like
v-tooltip
is great for that. Is there a way to popper like that? The vue wrapper I found did't seem to support it.Does prettier use the
.editorconfig
? It should be fine that case to format everything. Would like to move all formatting then to a different PR though.Probably yeah. We will probably miss some things but that should be alright.
I don't think so. Will give it another quick review and then merge it.
Thanks!
We would need to build it, but should be really straightforward.
Prettier respects eslint so I guess also editorconfig.
If you have never worked with prettier it will feel strange at first. After a bit it feels really backwards without it.