Introducing Themability & Vuetify #319
Conversation
|
Theming works really nice
|
|
@scramb mind reformatting the PR description ? The PR subject should be a text that could be included as is in a changelog. The first post of the PR should explain what the PR is about, and can include "fixes XYZ", etc. From reading the code it seems to load the theme JSON but not sure what it does with it and why it works. A summary would be nice. Thanks! @DeepDiver1975 introduce PR template ? |
scramb
changed the title
👍 |
I adjusted title and description of the PR |
|
github templates are here #322 |
|
Will it work with separate apps ? Does the theme need to be recompiled for every app or can it be used directly ? |
src/pages/error.vue
Outdated
| :src="configuration.theme.logo.big" | ||
| width="50%" | ||
| :aspect-ratio="1.96"> | ||
| <v-flex class="theme--light grey lighten-2 pa-5 ma-3 center-dialog" xs4> |
There was a problem hiding this comment.
"pa-5" and "ma-3" are these hard-coded values ?
what if we want to change the theme to have a different padding for these kind of components ?
might make more sense to have something like "pa-small", "pa-medium", "pa-large"
There was a problem hiding this comment.
https://vuetifyjs.com/en/layout/spacing - this are the spacing helpers - they can be used like {property}{direction}-{size}
There was a problem hiding this comment.
As I understand, in some cases one needs to use spacing helpers on top of existing predefined padding/margin defaults. But here I'm surprised that we have to use it that often.
So spacings are not defined as part of components ? Does v-btn have default paddings as part of the theme or do I need to explicitly set them every time I define a button ?
There was a problem hiding this comment.
v-btn got a default spacing which can be used.
There was a problem hiding this comment.
ok cool, so there are defaults.
should we rather adjust "pa-2" to be the default on "h2", "v-flex" and "v-btn" in the components below to avoid having to repeat it that often ?
my concern is mostly about having a too high reliance on such classes instead of having sensible defaults
There was a problem hiding this comment.
We could set this globally and override if needed i adjusted this
There was a problem hiding this comment.
I guess we will see cleaner classes as soon as the whole code base is migrated and uikit is not spitting in classes anymore ....
There was a problem hiding this comment.
as far as I understand, UIkit should not interfere with existing elements as it's "opt-in", you need to explicitly set the classes. So I don't think it''s a side effect.
You guys have decided to go with "pa-2" for whatever reason, maybe it was visually fitting. If possible it would be good to bring this to the default as soon as possible as I'm afraid that the markup becomes unmaintainable if we start adding such exceptions everywhere.
I'm fine merging this PR first then please address this as next task.
The theme is injected into vue instance and can be used from the apps aswell. Since we're loading the config from vuex we don't need to recompile |
Description
Introduced themability with vuetify and built example loginPage. Theme will be selected as before from
config.jsonby name.themeName.jsoninphoenix/themesfolder will be loaded to vuex store to make it accesible by every component.Material-Icons and Vuetify get bundled with the inline styles by webpack to one core.css file.
Related Issue
Motivation and Context
This PR could resolve all in #1 requested features and will help us to build a straight forward themeable design.
How Has This Been Tested?
yarn run watch fe.serv3r.cloud:8300on localhost connected against ownCloud X running on a dedicated machine.distoutput run with python as web-server connected against same ownCloud XScreenshots:
Types of changes
Checklist: