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

Add current theme higher priority than for Morpheus. #323

Merged
merged 3 commits into from Jun 30, 2014

Conversation

czolnowski
Copy link
Contributor

Adding this line gave higher priority for third-party theme, but doesn't cause problems described in this commit:
b06b118

Maybe this will fix bug with third-party themes! :)

@mattab
Copy link
Member

mattab commented Jun 19, 2014

as far as I understand the themes inherit from Morpheus so Morpheus should be loaded first and then other themes should be loaded. If there is a good reason to load current theme before Morpheus, maybe you can give me an example?

@mattab mattab closed this Jun 19, 2014
@mattab mattab reopened this Jun 19, 2014
@czolnowski
Copy link
Contributor Author

As I tested - this change should prioritise third-party theme which is currently active and move this theme less file at the end of final css file.

I'll test this again and confirm this behavior. Please, don't close this PR before my confirmation.

@mattab
Copy link
Member

mattab commented Jun 19, 2014

Sounds good! Have a good holiday
related ticket is http://dev.piwik.org/trac/ticket/5345

@mattab
Copy link
Member

mattab commented Jun 24, 2014

@czolnowski any update? maybe this needs to go in 2.4.0 so looking forward to it.

@czolnowski
Copy link
Contributor Author

https://www.dropbox.com/s/orxak0l8fdutrri/323_PR.png
@mattab: Please, take a look at this screen. It's xdebug breakpoint from UIAssetCatalogSorter:46. In this line you have sorted and ready to merge less files. First console shows files order before PR and second after PR. On second console you can see that custom theme (PROClean) is at the end of files array. Everything looks great, but... This is theme from piwik 2.3.x. And it's overwriting all files from Morpheus. I'm not sure how it will works without these overwriting.
Morpheus theme file should be on index 50, but I'm not sure why piwik put them on index ~5. (it is starting index for morpheus on my instance, so maybe piwik doesn't recognize this in sorting process?)
I'm open for every suggestion. Would be great if this change will be available in 2.4.0 release!

@mattab
Copy link
Member

mattab commented Jun 24, 2014

Are you seeing the custom theme loaded before Morpheus? That would be a bug.

If the custom theme is loaded after Morpheus, then it's working.

@czolnowski
Copy link
Contributor Author

"Are you seeing the custom theme loaded before Morpheus? That would be a bug." - this is behavior in current master.

"If the custom theme is loaded after Morpheus, then it's working." - this is behavior with my PR applied.

@quba
Copy link
Contributor

quba commented Jun 24, 2014

But one thing to improve would be to move Morpheus just one step before custom theme. Now I think it's incorrect, because Morpheus loads before other Piwik CSS files.

@czolnowski
Copy link
Contributor Author

@mattab: Please, take a look now. Morpheus and custom theme working together. If custom theme is enabled then order is: Morpheus -> Custom theme. And this is backward compatible.

@czolnowski czolnowski closed this Jun 24, 2014
@czolnowski czolnowski reopened this Jun 24, 2014
@mattab
Copy link
Member

mattab commented Jun 24, 2014

Cheers I've tested quickly and looks correct. I made smalll suggestion: czolnowski#2
please merge and then i'll merge this one!

@czolnowski
Copy link
Contributor Author

Hi @mattab. I've add comment to your patch: czolnowski#2 (comment)

@mattab
Copy link
Member

mattab commented Jun 26, 2014

Ok I understand now... LESS compiler does not work as we expected http://stackoverflow.com/questions/15383869/overriding-bootstrap-less-variables-after-import

Maybe we have to fork and extend, or inherit the lessC compiler? The new feature would be to allow defining variables in later content, and that the last variable value would be used.

For this maybe we could detect all variables used in any of the files and imported files, then move those variables up and first in the Less merged file, and then compile the less file and less would replace variable names by their latest defined value. Thoughts?

@mattab
Copy link
Member

mattab commented Jun 30, 2014

Here is my test procedure:

  • create a theme with: ./console generate:theme
  • in your theme/stylesheets/_colors.less set:
@theme-color-brand:                    #000;
@theme-color-text:                     #000;
@theme-color-link:                     #000;
  • delete cached styles $ rm tmp/assets/* -f
  • activate theme ./console core:plugin activate Testtheme2
  • in config file I put the Plugins[]=Testtheme2 before Plugins[]=Morpheus
  • I load Piwik and the colors overrides are applied

This works without and with the pull request.

mattab pushed a commit that referenced this pull request Jun 30, 2014
Add current theme higher priority than for Morpheus.
@mattab mattab merged commit 0e870c1 into matomo-org:master Jun 30, 2014
@mattab
Copy link
Member

mattab commented Jun 30, 2014

Ok I understand now... LESS compiler does not work as we expected

well actually... @tsteur said that when we load the theme files last, it should work and overwrite all variables defined before. If you find that this is not true please send us a test theme and steps to reproduce the issue and we will investigate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants