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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Theme preview tile for OS themes #3387

Merged
merged 6 commits into from May 19, 2021

Conversation

roguib
Copy link
Contributor

@roguib roguib commented May 15, 2021

Closes #3358 馃帀

What I did?

  1. I've extracted the svg that is used to preview the theme into a function named renderThemePreview.
  2. Then I invoked renderThemePreview under the select dropdown used for selecting the theme if the option use OS color scheme is enabled.

image

From the feature request:

Show a preview tile directly below each drop-down, only when Use OS color scheme is enabled.

I decided not to hide the preview tile and instead rendered it disabled since it was hard to notice that the preview tile appeared on the bottom of the select just by looking at the scrollbar. Here you can find the problem I'm trying to describe:

screen-capture.64.mp4

The final implementation looks as follows:

screen-capture.65.mp4

Let me know if you see anything that can be improved, I'll change it ASAP.

@roguib
Copy link
Contributor Author

roguib commented May 15, 2021

Not sure about the failing test in macOS since I can't test the application in that environment but it seems it's not reproducible in Windows/Linux. Do you know what could be the cause?

@develohpanda
Copy link
Contributor

develohpanda commented May 17, 2021

Not sure about the failing test in macOS since I can't test the application in that environment but it seems it's not reproducible in Windows/Linux. Do you know what could be the cause?

Seemed to be a smoke test timeout, these can happen sometimes. I have re-run 馃憤馃徑

@develohpanda develohpanda self-requested a review May 17, 2021 01:40
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Looks to be working, thanks for the PR! 馃憦馃徑

@develohpanda
Copy link
Contributor

From the feature request:

Show a preview tile directly below each drop-down, only when Use OS color scheme is enabled.

I decided not to hide the preview tile and instead rendered it disabled since it was hard to notice that the preview tile appeared on the bottom of the select just by looking at the scrollbar. Here you can find the problem I'm trying to describe:

Seems fine to me for the time being. We do have another task to bring the OS color scheme selector above the list of themes (instead of below, as it is now), and in that case hiding the previous would work better.

I pulled this branch locally and did a quick experiment with moving the selector and changing the behavior slightly, but will need some design feedback before going ahead with the following. This is unrelated to your PR though, which can go through regardless (pending another review/QA).

2021-05-17 15 26 37

@roguib
Copy link
Contributor Author

roguib commented May 17, 2021

Thanks for your review @develohpanda! I'm going to have a look at the issues page and start working on another issue.

Copy link
Contributor

@reynolek reynolek left a comment

Choose a reason for hiding this comment

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

LGTM from prod -- I love the feel of instant results.

@develohpanda develohpanda merged commit 3972bb5 into Kong:develop May 19, 2021
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.

Make it easier to see which theme is selected for OS themes
3 participants