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

Jump to bottom

Regression test for themes #8015

Merged
merged 8 commits into from Jun 1, 2015
Merged

Regression test for themes #8015

merged 8 commits into from Jun 1, 2015

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented May 28, 2015

Themes were broken because of new code now affected by this existing bug: leafo/lessphp#302. I bumped to v0.5.0 and checked the changelog, it was one of the 2 only significant change. The only other significant change is that syntax errors in Less now throw an exception (which allowed to fix a few existing bugs in our Less files). I think it's good to have (definitely help in when developing, I know I've raged a lot against mixins like .box-shadow() that weren't doing working without any explanation), however it might break plugins that have errors in their less files. Do we consider that a problem or a very small risk (for example we only had 2 Less bugs in our whole codebase)?

I added a UI test that checks that themes work so that we avoid any regression in the future.

Edit: FYI I removed the .box-shadow mixins for the following reasons:

They are useless since box-shadow is supported in all browsers except IE 8 (in which using a prefix doesn't help anyway).

@mnapoli
Update to lessphp 0.5.0 to get the fix for leafo/lessphp#302
808b351 
See leafo/lessphp#302 and http://stackoverflow.com/questions/15426256/override-less-variables
@mnapoli
Removed the .box-shadow mixins as they are useless and broken (break …
7974635 
…compiling to CSS)

They are useless since box-shadow is supported in all browsers except IE 8 (in which using a prefix doesn't help anyway).
@mnapoli
Fix: unknown Less variable being used
bdb1fd6 
I removed the line entirely:

- I couldn't find the matching variable (was most likely removed since then)
- if the line was broken before (silently ignored) then it's like it didn't exist
@mnapoli
Added UI test for themes
1aeed35 
This test enables the example theme and checks that it works.
@mnapoli mnapoli added c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. 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 28, 2015
@mnapoli mnapoli added this to the 2.14.0 milestone May 28, 2015
This was referenced May 28, 2015
Closed
Closed
@diosmosis
Copy link
Member

Are there any themes other than Morpheus that we maintain (I know there used to be PleineLune, but I think we stopped maintaining it)? If so, it would be good if we added UI tests for them (if we don't have them already).

@mattab
Copy link
Member

mattab commented May 29, 2015

however it might break plugins that have errors in their less files. Do we consider that a problem or a very small risk (for example we only had 2 Less bugs in our whole codebase)?

+1 - I think it's not a problem as it is only small risk

I added a UI test that checks that themes work so that we avoid any regression in the future.

👍 for the idea. The test enables theme ExampleTheme which looks the same as the default theme. Will this new test still help us detect a regression?

@mattab
Copy link
Member

mattab commented May 29, 2015

Are there any themes other than Morpheus that we maintain (I know there used to be PleineLune, but I think we stopped maintaining it)?

PleineLune was deprecated, and there are no other theme that we need to maintain (at the moment) .

@mnapoli
Copy link
Contributor Author

mnapoli commented May 29, 2015

@mattab have a look at this: 7974635#commitcomment-11414730

@diosmosis
Copy link
Member

What about taking a screenshot of the UI demo page? Will the theme be applied there?

Also, do you intend to customize the theme more (ie, use more variables) or is this it?

@mnapoli
Copy link
Contributor Author

mnapoli commented May 31, 2015

What about taking a screenshot of the UI demo page? Will the theme be applied there?

Yes it should definitely apply there. I've added a test for that page.

Also, do you intend to customize the theme more (ie, use more variables) or is this it?

No, we could decide to test every variable but right now I don't think it's very urgent/a priority. This test is mostly about checking that it works (e.g. on master themes don't work)

@diosmosis
Copy link
Member

Screenshot tests are failing, I think phantomjs doesn't support box-shadow. Not sure how to fix this w/o putting the macro back.

@mnapoli
Copy link
Contributor Author

mnapoli commented May 31, 2015

Following discussion, TODO:

  • remove the button shadows (this PR actually fixed the shadow which wasn't rendered before because the mixin was broken)
  • button shadows will be added in New design for buttons #7896 anyway so there should be no UI change in this PR

@mnapoli mnapoli added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels May 31, 2015
@mnapoli
#8015 Remove box-shadow rules that were previously broken
9c1b052 
Fixing those rules in #8015 changed some UI components as it added a shadow (which wasn't showing before because broken). With this commit, those shadows are now removed so that #8015 doesn't introduce any visual change.
@mnapoli
Fix UI test
e79f56a
@mnapoli
#8015 Re-add some box-shadows that were actually working to make sure…
01afd50 
… the pull request introduces absolutely no visual change
@mnapoli mnapoli added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Jun 1, 2015
@mnapoli
Copy link
Contributor Author

mnapoli commented Jun 1, 2015

Shadows and tests are fixed, this PR introduces no visual change: http://builds-artifacts.piwik.org/ui-tests.fixing-themes/12946.7/screenshot-diffs/diffviewer.html

Ready for review and merge.

diosmosis added a commit that referenced this pull request Jun 1, 2015
@diosmosis
Merge pull request #8015 from piwik/fixing-themes
1012773 
Add regression UI test for themes. Uses ExampleTheme to test theming still works (in general).
@diosmosis diosmosis merged commit 1012773 into master Jun 1, 2015
@diosmosis diosmosis deleted the fixing-themes branch June 1, 2015 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. 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
Milestone
2.14.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mnapoli @diosmosis @mattab