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

fix: make space to left of dashboard items scrollable [DHIS2-10138] #1523

Merged
merged 7 commits into from
Feb 12, 2021

Conversation

jenniferarnesen
Copy link
Collaborator

@jenniferarnesen jenniferarnesen commented Feb 9, 2021

Fixes:

  • use padding instead of margin so that scrolling works when scrolling the empty space along the left of the items
  • migrate to consistent css classes language (wrapper => container)
  • add buttons for both small and wide view and show/hide using css. This avoids the need for javascript to determine which button to show and will behave more instantaneously
  • remove position:fixed on the action bar as it isn't needed (this was a relic from using the view dashboard as a template)
scrolling.mov

Copy link
Contributor

@martinkrulltott martinkrulltott left a comment

Choose a reason for hiding this comment

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

LGTM 🏅 the actual change was so good I had to move out of scope to find things to comment 😆 Anyway, it's good to merge as is, just couldn't help myself to write comments while I was reviewing 🍻

const { width } = useWindowDimensions()
const isSmall = isSmallScreen(width)
const getExitPrintButton = isSmall => {
const className = isSmall ? classes.buttonSmall : classes.buttonWide
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what the best naming convention is, but could we possibly use small - large or maybe narrow - wide? small - wide sound like two different properties to me (however your English is better than mine so I could be wrong).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also consider moving the ternary operator directly into the className={ ... } to reduce the footprint a bit.

@media print {
.wrapper {
.container {
background-color: white;
height: auto !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably out of scope, but I noticed these !important. media -> class should override any class selector, so are these really necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've been starting to review a lot of the css and I'll make a note to look at this one. Thanks!

background-color: white;
height: auto !important;
overflow: visible !important;
margin-left: 0px;
padding-left: 0px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also not in scope, and it has no production code impact, but my personal preference is to use 0 instead of 0px. This is just my OCD speaking, so disregard this if you have a different opinion 😄

@jenniferarnesen jenniferarnesen enabled auto-merge (squash) February 12, 2021 16:54
@jenniferarnesen jenniferarnesen merged commit 3305aa3 into master Feb 12, 2021
@jenniferarnesen jenniferarnesen deleted the fix/print-scrollable-left-margin branch February 12, 2021 16:59
dhis2-bot added a commit that referenced this pull request Feb 12, 2021
## [31.10.9](v31.10.8...v31.10.9) (2021-02-12)

### Bug Fixes

* add css class for expanded control bar ([286c1ae](286c1ae))
* clean dashboard bar height calculation function ([71bb64d](71bb64d))
* make space to left of dashboard items scrollable [DHIS2-10138] ([#1523](#1523)) ([3305aa3](3305aa3))
* remove inline style calculation height for small, dashboardbar chip container and controlbar ([99dea63](99dea63))
* update dashboard bar snapshot ([1caac43](1caac43))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 31.10.9 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants