feature/move-styles-to-components #874

Merged
konrad merged 45 commits from dpschen/frontend:feature/move-styles-to-components into main 2021-10-21 19:11:18 +00:00
Member

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

  • 🟢 some components like color-picker and date-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.
  • 🟡 Some components contain now styles for components that are deeply nested. Styles should always apply on the current component only. In some cases this makes it necessary to use Deep-Selectors (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 at components/input/datepicker.vue. The ::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 the inner-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). This was wrong. I didn't understand :deep() completely. The selector seems to work.
# 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 - [x] 🟢 some components like `color-picker` and `date-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 https://kolaente.dev/vikunja/frontend/commit/7a95d08e1e08c8bb88ae67c98a230ab9c6800763. - [ ] 🟡 Some components contain now styles for components that are deeply nested. Styles should always apply on the current component only. In some cases this makes it necessary to use Deep-Selectors (~~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 at `components/input/datepicker.vue`. ~~The `::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 the `inner-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).~~ *This was wrong. I didn't understand `:deep()` completely. The selector seems to work.*
dpschen added the
kind/feature
label 2021-10-18 13:16:29 +00:00
konrad was assigned by dpschen 2021-10-18 13:16:29 +00:00
dpschen added 38 commits 2021-10-18 13:16:30 +00:00
ce94594a75
feat: merge attachment styles with component
also add bounce animation that is just used there
1f01567a99
feat: merge kanban.scss styles with component
.ghost-task-drop class was removed because it was used nowhere.
fcc1ba819d
feat: divide most list.scss styles into components
- list-card.vue and the Home.vue
- listSearch.vue
- topNavigation.vue
- EditTeam.vue
- List.vue
- ShowList.vue
01f5c9559c
feat: divide most tasks.scss styles into components
- ShowTasks.vue
- List.vue
- defer-task.vue
- edit-task.vue
- Kanban.vue, relatedTasks.vue and singleTaskInView.vue
2596fd98b4
feat: divide most content.scss styles into components
- contentAuth
- contentNoAuth.vue
- Login.vue
- button.vue
- comments.vue
359dc542f7
feat: divide most navigation.scss styles into components
- navigation.vue and topNavigation.vue
- contentAuth.vue
continuous-integration/drone/pr Build is passing Details
e70d6484f8
feat: add FIXME comments
konrad approved these changes 2021-10-19 19:20:45 +00:00
konrad left a comment
Owner

I think this is a good chance to format all css to use tabs instead of spaces consistently - could you do that?

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)

Did you put this seperately from the other style tag to prevent importing and prefixing these global styles in the scope? (possibly multiple times)
Author
Member

Not sure if I 100% understand the question. But I'll just try to explain why there are two style tag in this component:

  1. 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.

  2. The style with the scoped attribute contains the styles that get scoped by 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-loader both of these kind of styles will get the
compiled 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:

.offline {
  background: url('@/assets/llama-nightscape.jpg') no-repeat center;
  background-size: cover;
  height: 100vh;
}

.offline-message {
  text-align: center;
  position: absolute;
  width: 100vw;
  bottom: 5vh;
  color: $white;
  padding: 0 1rem;
}

h1 {
  font-weight: bold;
  font-size: 1.5rem;
  text-align: center;
  color: $white;
  font-weight: 700 !important;
  font-size: 1.5rem;
}

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 :)

Not sure if I 100% understand the question. But I'll just try to explain why there are two style tag in this component: 1) 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. 2) The style with the `scoped` attribute contains the styles that get scoped ~~by 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-loader~~ both of these kind of styles will get the compiled 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](https://developer.mozilla.org/en-US/docs/Web/CSS/Specificity): ```scss .offline { background: url('@/assets/llama-nightscape.jpg') no-repeat center; background-size: cover; height: 100vh; } .offline-message { text-align: center; position: absolute; width: 100vw; bottom: 5vh; color: $white; padding: 0 1rem; } h1 { font-weight: bold; font-size: 1.5rem; text-align: center; color: $white; font-weight: 700 !important; font-size: 1.5rem; } ``` 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!

In reality every component imports them again on their own.

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?

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:

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.

Thanks for clarifying! > In reality every component imports them again on their own. 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? > 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: 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.
konrad marked this conversation as resolved
@ -5,7 +5,6 @@
:disabled="disabled || null"
:id="checkBoxId"
@change="(event) => updateData(event.target.checked)"
style="display: none;"

Good catch.

Good catch.
dpschen marked this conversation as resolved
@ -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?

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.

I think the class in `userTeam.vue` is using the bulma styling though so it *should* be save to remove this.
Author
Member

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?

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?
Author
Member

Done

Done
dpschen marked this conversation as resolved
Owner

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 manual

Are these variables defined with !default? Then we could just define them in "our" scss variables config file instead, couldn't we?

> 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 manual Are these variables defined with `!default`? Then we could just define them in "our" scss variables config file instead, couldn't we?
Owner

There's a few things I noticed:

  1. In the menu:

image

  1. Spacing between tasks and labels:

image

  1. Spacing and alignment on kanban cards:

image

  1. Button background on user menu:

image

  1. 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)

  2. The list menu on the top title bar needs some spacing:

image

I think we should fix at least some of those before merging this.

Maybe using a tool like percy could a good idea here?

There's a few things I noticed: 1. In the menu: ![image](/attachments/1caf27b6-17b0-4ab1-bb19-878da88199e7) 2. Spacing between tasks and labels: ![image](/attachments/f524f27c-4481-439f-bee6-553e88b65589) 3. Spacing and alignment on kanban cards: ![image](/attachments/c7694ef6-59a1-44bd-a365-16cb4a4eeea8) 4. Button background on user menu: ![image](/attachments/b91a37d8-e6a4-4ae0-afed-42b88e6aa988) 5. 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`) 6. The list menu on the top title bar needs some spacing: ![image](/attachments/4ae31279-46c6-4895-bea6-35d9489bdf89) I think we should fix at least some of those before merging this. Maybe using a tool like [percy](https://percy.io/) could a good idea here?
Owner

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)

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)
Author
Member

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 :)

> 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](https://popper.js.org/). 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](https://popper.js.org/docs/v2/#why-not-use-pure-css). 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 :)
Author
Member

I think this is a good chance to format all css to use tabs instead of spaces consistently - could you do that?

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.

> I think this is a good chance to format all css to use tabs instead of spaces consistently - could you do that? 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.
Author
Member

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 manual

Are these variables defined with !default? Then we could just define them in "our" scss variables config file instead, couldn't we?

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 the additionalData 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.

> > 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 manual > > Are these variables defined with `!default`? Then we could just define them in "our" scss variables config file instead, couldn't we? 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 the `additionalData` 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](https://kolaente.dev/dpschen/frontend/src/branch/feature/move-styles-to-components/src/styles/variables/_index.scss#L6) 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](https://kolaente.dev/dpschen/frontend/src/branch/feature/move-styles-to-components/src/styles/global.scss#L11) 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: https://kolaente.dev/vikunja/frontend/commit/7a95d08e1e08c8bb88ae67c98a230ab9c6800763.
Author
Member

Maybe using a tool like percy could a good idea here?

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?

> Maybe using a tool like percy could a good idea here? 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?
Author
Member

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.

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.
dpschen added 1 commit 2021-10-20 10:20:24 +00:00
continuous-integration/drone/pr Build is passing Details
6576a6214a
feat: move some form.scss styles to button.vue
Author
Member

Tracking those fixes here:

  • 1. In the menu
  • 2. Spacing between tasks and labels
  • 3. Spacing and alignment on kanban cards
  • 4. Button background on user menu
    => 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.
  • 5. 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)
    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 🎉
  • 6. The list menu on the top title bar needs some spacing:
### Tracking those fixes here: * [x] 1. In the menu * [x] 2. Spacing between tasks and labels * [x] 3. Spacing and alignment on kanban cards * [x] 4. Button background on user menu *=> 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.* * [x] 5. 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`) **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 🎉 * [x] 6. The list menu on the top title bar needs some spacing:
dpschen added 1 commit 2021-10-20 11:13:03 +00:00
continuous-integration/drone/pr Build is passing Details
fe7880e864
fix: user dropdown-trigger background
dpschen added 2 commits 2021-10-20 12:34:04 +00:00
dpschen added 1 commit 2021-10-20 12:47:22 +00:00
continuous-integration/drone/pr Build is passing Details
d6b168449d
fix kanban height calculation with hack
dpschen added 1 commit 2021-10-20 13:05:17 +00:00
continuous-integration/drone/pr Build is passing Details
7a95d08e1e
fix: use $shadow variable directly
By using the $shadow variable directly that defines the $card-shadow variable in bulma we don't need to import card.scss
dpschen added 1 commit 2021-10-20 16:45:41 +00:00
continuous-integration/drone/pr Build is passing Details
6b447a1ba3
feat: add Done component
Author
Member

@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.

@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.
Owner

I have some good experience with popper.js. It's basicalally a pure battle tested tooltip positioning libary.

Instead it's relly easy to build a simple implmentation on your own. Maybe I have somewhere still a component. Will check :)

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.

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.

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.

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?

Probably yeah. We will probably miss some things but that should be alright.

> I have some good experience with popper.js. It's basicalally a pure battle tested tooltip positioning libary. > Instead it's relly easy to build a simple implmentation on your own. Maybe I have somewhere still a component. Will check :) 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. > 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. 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. > 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? Probably yeah. We will probably miss some things but that should be alright.
Owner

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 don't think so. Will give it another quick review and then merge it.

> 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 don't think so. Will give it another quick review and then merge it.
konrad merged commit 5dcb1dd449 into main 2021-10-21 19:11:18 +00:00
konrad deleted branch feature/move-styles-to-components 2021-10-21 19:11:18 +00:00
Owner

Thanks!

Thanks!
Author
Member

Is there a way to popper like that?

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.

> Is there a way to popper like that? 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.
This repo is archived. You cannot comment on pull requests.
No description provided.