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

#2287: replace selection list by simple list for change theme dialog #2338

Merged
merged 7 commits into from
Apr 8, 2019

Conversation

glefi
Copy link
Collaborator

@glefi glefi commented Mar 29, 2019

Closes #2287

@codecov-io
Copy link

codecov-io commented Mar 29, 2019

Codecov Report

Merging #2338 into master will decrease coverage by 0.09%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2338     +/-   ##
=========================================
- Coverage   91.45%   91.35%   -0.1%     
=========================================
  Files         103      106      +3     
  Lines        1392     1446     +54     
=========================================
+ Hits         1273     1321     +48     
- Misses        119      125      +6
Impacted Files Coverage Δ
src/components/WorkspaceMenu.js 83.33% <0%> (ø) ⬆️
src/lib/KeyHelper.js 100% <100%> (ø)
src/components/ChangeThemeDialog.js 66.66% <66.66%> (ø)
src/components/WorkspaceSelectionDialog.js 90% <80%> (-10%) ⬇️
src/lib/ListKeyboardNavigation.js 94.87% <94.87%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 636d712...d540545. Read the comment docs.

@ggeisler
Copy link
Collaborator

@glefi Nice start on this. I'm not sure if this PR is intended to be complete or a work-in-progress but here are a couple of minor issues that should be updated, in case you are not already aware of them:

Some things that don't match the component demo at https://material-ui.com/demos/dialogs/#simple-dialogs but should:

  • Hover highlighting as you move over the different theme options
  • Cursor should be the hand cursor when highlighting any part of the theme options; currently it shows a pointer cursor when hovering over the icon and changes to a selection cursor when hovering over the theme option text
  • The dialog should close automatically when selecting an option; currently the theme changes when selecting an option but the dialog doesn't close until you click again

Also, the dialog doesn't seem accessible via keyboard?

@glefi glefi force-pushed the 2287-update-the-settings-dialog-to-be-change-theme branch from aec4732 to 8db6678 Compare April 2, 2019 13:18
@glefi glefi changed the title fixes #2287: replace selection list by simple list for change theme dialog [WIP] #2287: replace selection list by simple list for change theme dialog Apr 3, 2019
@glefi glefi force-pushed the 2287-update-the-settings-dialog-to-be-change-theme branch from 54926aa to d540545 Compare April 5, 2019 14:27
@glefi glefi changed the title [WIP] #2287: replace selection list by simple list for change theme dialog #2287: replace selection list by simple list for change theme dialog Apr 5, 2019
@glefi
Copy link
Collaborator Author

glefi commented Apr 5, 2019

@ggeisler Thanks for your feedback. The dialog should now meet all the requirements. It would be nice if you could take another look at it.

@ggeisler
Copy link
Collaborator

ggeisler commented Apr 5, 2019

Yep, the Change theme dialog looks good to me.

@camillevilla camillevilla self-requested a review April 5, 2019 22:49
Copy link
Member

@camillevilla camillevilla left a comment

Choose a reason for hiding this comment

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

Looks good to me, @glefi!
Screen Shot 2019-04-05 at 4 11 35 PM

I did notice some stuff for the Select Workspace Type dialog ticket #2288 in here as well. I wasn't sure if that was a prerequisite for #2287 (which I see you're also assigned to), but I'm definitely not complaining about more cats in Mirador...

I'm approving this PR for now. Feel free to merge it yourself when you see this!

@ggeisler
Copy link
Collaborator

ggeisler commented Apr 6, 2019

Just to be clear, my comments above on this PR were for only the Change theme dialog, not the Select workspace type dialog.

@glefi I assume you know that the Change workspace type dialog still needs:

  • Adding the SVG icons I pointed you to
  • Showing highlighting for the currently active workspace type

The latter issue does work in the Change theme dialog but last I saw when you open the Change workspace type dialog it does not show any highlighted for the currently selected type.

@glefi glefi merged commit e91995b into master Apr 8, 2019
@cbeer cbeer deleted the 2287-update-the-settings-dialog-to-be-change-theme branch April 8, 2019 15:00
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.

4 participants