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.

Sign up for GitHub

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

Use jss to style Gallery view so it can be more easily themed via MUI. #2291

Merged
merged 1 commit into from
Mar 22, 2019
Merged

Use jss to style Gallery view so it can be more easily themed via MUI. #2291

merged 1 commit into from
Mar 22, 2019

Conversation

jkeck
Copy link
Member

@jkeck jkeck commented Mar 21, 2019

Closes #2251

GalleryCanBeThemed

NowWithTheme

@jkeck jkeck force-pushed the 2251-gallery-theme branch 2 times, most recently from 2fa8ce4 to 98b1132 Compare March 21, 2019 23:59
@codecov-io
Copy link

codecov-io commented Mar 22, 2019 •
edited
Loading

Codecov Report

Merging #2291 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2291   +/-   ##
=======================================
  Coverage   96.09%   96.09%           
=======================================
  Files          94       94           
  Lines        1178     1178           
=======================================
  Hits         1132     1132           
  Misses         46       46
Impacted Files Coverage Δ
src/components/GalleryView.js 85.71% <100%> (-4.29%) ⬇️
src/components/WindowSideBarButtons.js 100% <0%> (ø) ⬆️

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 859774b...eed124a. Read the comment docs.

jkeck
jkeck commented Mar 22, 2019
View reviewed changes
src/containers/GalleryView.js Outdated
margin: '10px 5px',
maxWidth: '100px',
overflow: 'hidden',
padding: '5px',
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if the padding/margins here should be using theme variables or not (I'm just moving it from index.scss here). If anybody thinks that it would make sense to use the spacing vars or anything, let me know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd certainly be in favor of using the theme spacing (unless there's a strong reason we need 1px more spacing in gallery view than everywhere else we're adding padding + margins)?

@cbeer cbeer merged commit 667eec1 into master Mar 22, 2019
@cbeer cbeer deleted the 2251-gallery-theme branch March 22, 2019 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbeer cbeer cbeer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jkeck @codecov-io @cbeer