Skip to content
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

Reorganized less variables for simpler theming #7951

Merged
merged 4 commits into from May 25, 2015
Merged

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented May 20, 2015

See #7876 for background

We now have theme.less and theme-advanced.less to separate simple variables for plugin developers from advanced variables.

In Morpheus:

base.less (unchanged)
main.less (was previously theme.less)
theme.less -> variables for themes (was previously `less/_variables.less`)
theme-advanced.less -> advanced variables for themes

That makes Morpheus have the same layout as other themes (i.e. theme.less contains the theme variables), which I think is great for developers that could be previously confused with the file names.

By the way I took great care to make sure git moves files and not deletes/creates them, but the diff here doesn't seem to show that which is a shame (in phpstorm files are correctly moved) -_-

@mnapoli mnapoli added c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels May 20, 2015
@mnapoli mnapoli added this to the 2.14.0 milestone May 20, 2015

@theme-color-border: @color-silver-l80;

@theme-color-menu-contrast-text: @theme-color-text-lighter;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theme-color-menu-* and @theme-color-widget-* should be in theme.less I think. Those are specifically exposed API's/variables to allow developers to change those. Or with other words those are the things we identified that companies might want to change if they want to adjust Piwik to their CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, last time though you argued that most themes are about replacing only the main Piwik colors with the one of the company/host/agency… The menu and widget variables are linked to other variables so they should adjust when we change the base color variables (i.e. text color + background color). Anyway I don't mind moving those variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure where I mentioned it, eg in #7876 (comment) I mentioned to put the menu and widget colors in theme.less. They are very important for theming. The @theme-color-background-* variables should be still in theme-advanced as they are "complicated" (whch I also mentioned here #7793 (comment) ).

I know they default to other variables but we want users to be able to just specifically change those widget and menu colors without affecting anything else to keep it as easy as possible. Just by changing those 5 menu and widget variables they will be able to create a nice theme according to their CI.

@tsteur
Copy link
Member

tsteur commented May 20, 2015

For me there are 2 things still unclear:

  1. How do we make clear that only variables in theme.less and theme-advanced.less are API and BC is guaranteed. We might need a new page under developer.piwik.org -> Reference? Maybe this one could be automatically created with the other pages? It could automatically detect a doc block for each variable and generate documentation based on this.
  2. How do we make sure we keep BC etc? Can we maybe create a UI test that activates a theme and takes only a very few screenshots eg of dashboard, a report page, an admin page to see whether we break anything and to make sure when adding new colors/css that it will work with this? (It is quite easy to break theming eg just by renaming the files eg because of https://github.com/piwik/piwik/blob/2.14.0-b1/core/AssetManager/UIAssetFetcher/StylesheetUIAssetFetcher.php#L16-L42 , we had this in the past)

Asking 1) here as we added a couple of new variables and I would not feel good about adding more variables without having this. We can create a new issue for automatically generating it, but then we should maybe create at least a developer documentation/reference page manually.

@mnapoli
Copy link
Contributor Author

mnapoli commented May 21, 2015

  1. We could update the developer documentation (i.e. link to those files), that's what I was thinking at least.
  2. Sounds like a good idea (to be done separately).

FYI I haven't added or removed variables in this PR, I have only reorganized them. And I believe since I started the redesign I have only added the @theme-color-border variable.

@mnapoli
Copy link
Contributor Author

mnapoli commented May 25, 2015

The UI test fail is a random failure: http://builds-artifacts.piwik.org/ui-tests.simpler-theme/12676.7/screenshot-diffs/diffviewer.html

Ready to be merged

@mattab
Copy link
Member

mattab commented May 25, 2015

Feedback:

  • Simpler theming! 👍
  • we need to update the Theming developer guide which references theme.less file http://developer.piwik.org/guides/theming
  • Do you think this potentially could break any of the two themes published on the Marketplace (and possibly other themes?) or is BC kept?

@mnapoli
Copy link
Contributor Author

mnapoli commented May 25, 2015

I have just renamed some files and moved less variables, so there should be no BC breaks.

Will update the developer documentation.

diosmosis added a commit that referenced this pull request May 25, 2015
Reorganized less variables for simpler theming. Moved definition of basic LESS variables to theme.less and advanced variables to theme-advanced.less (in Morpheus).
@diosmosis diosmosis merged commit ce03bfa into master May 25, 2015
@diosmosis diosmosis deleted the simpler-theme branch May 25, 2015 16:34
@tsteur
Copy link
Member

tsteur commented May 25, 2015

I have just renamed some files and moved less variables, so there should be no BC breaks.

We should still test it if possible. As mentioned earlier we had problems in the past with this just by renaming files eg because of
https://github.com/piwik/piwik/blob/2.14.0-b1/core/AssetManager/UIAssetFetcher/StylesheetUIAssetFetcher.php#L16-L42

mnapoli added a commit to matomo-org/developer-documentation that referenced this pull request May 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants