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

Allow switching between visual test themes easily #1198

Merged
merged 3 commits into from
Jun 19, 2019

Conversation

ashmaroli
Copy link
Contributor

So that one does not need to remember the correct theme names to test..

Copy link
Contributor

@pyrmont pyrmont left a comment

Choose a reason for hiding this comment

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

Sorry, @ashmaroli. This review might come off as overly negative. I can see you've done a lot of work here but I really don't think a custom selector is the right approach. I believe we should be aiming to keep these pages as simple and straightforward as possible.

spec/visual/templates/layout.erb Outdated Show resolved Hide resolved
spec/visual/templates/layout.erb Outdated Show resolved Hide resolved
spec/visual/templates/layout.erb Outdated Show resolved Hide resolved
@pyrmont pyrmont added the author-action The PR has been reviewed but action by the author is needed label Jun 18, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Jun 18, 2019

I should have added that, despite my negativity, I do very much like the goal of this PR. Being able to switch themes easily is a good idea.

@ashmaroli
Copy link
Contributor Author

I believe we should be aiming to keep these pages as simple and straightforward as possible.

Thank you for taking the time for reviewing this @pyrmont. I got carried away and forgot that this is not a consumer-facing page.. 😛

Nevertheless, I've pushed a minimal version.

@pyrmont pyrmont dismissed their stale review June 19, 2019 05:55

No longer necessary

@pyrmont
Copy link
Contributor

pyrmont commented Jun 19, 2019

@ashmaroli This is better but honestly, when I ran the PR on my system I didn't realise it was a selection control! And I knew that it was supposed to be one! That's cool in its own way but I think will prevent people from realising that there's a control there.

For mind, a default control is best in this situation. In the PR I submitted to your branch, I stripped out all of the CSS and just left the JavaScript (I almost wanted to add in a proper form element so that JavaScript wasn't required but I relented on that—at least for this PR).

I will say that it was great to be able to switch between the themes!

@ashmaroli
Copy link
Contributor Author

I also thought about having a navbar rendered instead of resorting to JavaScript. But that's boring! 😛

@pyrmont
Copy link
Contributor

pyrmont commented Jun 19, 2019

@ashmaroli Now you're talking my language :P

@pyrmont
Copy link
Contributor

pyrmont commented Jun 19, 2019

@ashmaroli Do you have anything else you want to add? I'll merge otherwise.

@ashmaroli
Copy link
Contributor Author

Do you have anything else you want to add?

No. The current state is fine.
Please proceed.

@pyrmont pyrmont merged commit 6fb22a5 into rouge-ruby:master Jun 19, 2019
@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Jun 19, 2019
@ashmaroli ashmaroli deleted the theme-selector branch June 19, 2019 06:38
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.

2 participants