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

fix: control bar refactor #1529

Merged
merged 46 commits into from
Feb 17, 2021
Merged

fix: control bar refactor #1529

merged 46 commits into from
Feb 17, 2021

Conversation

jenniferarnesen
Copy link
Collaborator

@jenniferarnesen jenniferarnesen commented Feb 11, 2021 •
edited
Loading

Fixes:

  • in small screen, make dashboard non-interactive when control bar is expanded (DHIS2-10436)
non-interactive.dashboard.when.cb.expanded.mov
  • in small screen control bar horiz scrollbar only shows half height (DHIS2-10417)

  • control bar now resizes instantly when moving between portrait and landscape (DHIS2-10418)
landscape.to.portrait.mov
  • control bar fixed while dashboard and header bar scroll in phone landscape orientation (DHIS2-10423)
    ** No video for this, but since the controlbar is no longer position fixed (except when expanded), it should behave better.

Refactor:

  • remove most height calculations for dashboard and control bar. This includes for the PrintDashboards. Use flexbox and margins instead.
  • separate control bar drag functionality into a DragHandle component
    ** Most code in DragHandle hasn't changed from its original form in the deleted ControlBar file
  • delete ControlBar and move functionality (related to userRows) to DashboardsBar together. Most height calcs removed, and instead using hardcoded row heights in the css.
  • Filter component is kind of complex now, as it renders both a Filter for sm (collapsed and expanded) and lg screens (controlled by css display).

@jenniferarnesen jenniferarnesen marked this pull request as draft February 11, 2021 15:44
@jenniferarnesen
fix: remove height calculations from print views
506596b
@jenniferarnesen
Merge branch 'master' into fix/no-interactive-below-expanded-ctrlbar
9777693
@jenniferarnesen
fix: new dashboard scrolling and expanded in < 480px height
7671e02
@jenniferarnesen
fix: use css for display/hide of not supported notice
ec6f22d
@jenniferarnesen
fix: clean up styles in ctrl bar
6c3ad8c
@jenniferarnesen
fix: use css to show/hide drag handle
7f9365b
@jenniferarnesen
fix: absolutely position showmore button
b99733c
@jenniferarnesen
fix: rename root to container for constistency
2c9db3f
@jenniferarnesen
fix: use css to set ctrl bar height
fac1de4
@jenniferarnesen
fix: controlbar content refactor to avoid using window width
9dd06d7
@jenniferarnesen
fix: show search icon in search area
524c94e
@jenniferarnesen
fix: only expanded or collapsed class at top level
3ee3172
@jenniferarnesen
fix: positioning for small collapsed
d16605e
@jenniferarnesen
fix: positioning for the search field
01b61fa
@jenniferarnesen
fix: use css instead of window dims for component cover
6c941fe
@jenniferarnesen
Merge branch 'master' into fix/no-interactive-below-expanded-ctrlbar
9a8c204
@jenniferarnesen
fix: positioning of the search button for small-collapsed view
67c4f6d
@jenniferarnesen
fix: position of search field in wide view
ce618c4
@jenniferarnesen
fix: position of search field in wide view
594d7b5
@jenniferarnesen jenniferarnesen marked this pull request as ready for review February 15, 2021 10:38
edoardo
edoardo approved these changes Feb 16, 2021
View reviewed changes
Copy link
Member

@edoardo edoardo left a comment

Choose a reason for hiding this comment

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

Big PR, but it seems ok.
One comment about the icons used, but they can be changed later in a separate PR.

src/components/ControlBar/ViewControlBar/ClearButton.js Show resolved Hide resolved
martinkrulltott
martinkrulltott reviewed Feb 16, 2021
View reviewed changes
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.

Generally looks good, just some minor details, see the individual comments

src/components/Dashboard/styles/PrintLayoutDashboard.module.css Outdated Show resolved Hide resolved
src/components/Dashboard/styles/ViewDashboard.module.css
display: none;
}

@media only screen and (max-height: 480px), only screen and (max-width: 480px) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like there's a lot of these media queries now, maybe we could extract the queries to consts, to just use an abstraction like e.g. @media var(--phone-landscape), var(--phone-portrait) instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That could be. I've never tried it actually, so I'll try now.

cypress/integration/ui/view_dashboard.feature Outdated Show resolved Hide resolved
cypress/integration/ui/view_dashboard/control_bar.js Outdated Show resolved Hide resolved
src/components/ControlBar/ViewControlBar/DashboardsBar.js Outdated Show resolved Hide resolved
src/components/ControlBar/ViewControlBar/styles/Content.module.css Outdated Show resolved Hide resolved
src/components/ControlBar/ViewControlBar/styles/Content.module.css Outdated Show resolved Hide resolved
src/components/ControlBar/ViewControlBar/styles/DashboardsBar.module.css Outdated Show resolved Hide resolved
src/components/ControlBar/ViewControlBar/styles/DashboardsBar.module.css Outdated Show resolved Hide resolved
src/components/ControlBar/ViewControlBar/styles/DragHandle.module.css Outdated Show resolved Hide resolved
martinkrulltott
martinkrulltott approved these changes Feb 17, 2021
View reviewed changes
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.

🥇

@jenniferarnesen jenniferarnesen merged commit 84dde66 into master Feb 17, 2021
@jenniferarnesen jenniferarnesen deleted the fix/no-interactive-below-expanded-ctrlbar branch February 17, 2021 09:44
dhis2-bot added a commit that referenced this pull request Feb 17, 2021
@dhis2-bot
chore(release): cut 31.11.1 [skip ci]
1de7809 
## [31.11.1](v31.11.0...v31.11.1) (2021-02-17)

### Bug Fixes

* control bar refactor ([#1529](#1529)) ([84dde66](84dde66))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 31.11.1 🎉

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
Reviewers

@cooper-joe cooper-joe cooper-joe approved these changes

@martinkrulltott martinkrulltott martinkrulltott approved these changes

@edoardo edoardo edoardo approved these changes

@Sharmyn28 Sharmyn28 Awaiting requested review from Sharmyn28

@janhenrikoverland janhenrikoverland Awaiting requested review from janhenrikoverland

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

Successfully merging this pull request may close these issues.
5 participants
@jenniferarnesen @dhis2-bot @edoardo @martinkrulltott @cooper-joe