Page MenuHomeSoftware Heritage
Differential D1583

Fix sidebar not opening first time
ClosedPublic
Actions

Authored by kalpitk on Jun 14 2019, 12:29 PM.

Details

Reviewers
vlorentz
anlambert
Group Reviewers
Reviewers
Commits
rDWAPPS208d01b1fe9d: Fix sidebar not opening first time
Summary

On devices with <992px width (sm, xs), sidebar doesn't open when swh-push-menu is clicked first time.
This was due to webapp-utils.js:5 setting collapseSidebar to false by default on all devices.

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kalpitk created this revision.Jun 14 2019, 12:29 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptJun 14 2019, 12:29 PM
swh-public-ci added a comment.Jun 14 2019, 12:33 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/490/ for more details.

Harbormaster completed remote builds in B6209: Diff 5235.Jun 14 2019, 12:33 PM
vlorentz added a subscriber: vlorentz.Jun 14 2019, 2:15 PM

This needs a test (after D1577 lands)

vlorentz requested changes to this revision.Jun 14 2019, 2:15 PM
This revision now requires changes to proceed.Jun 14 2019, 2:15 PM
vlorentz added a comment.Jun 14 2019, 2:17 PM

Where is the value "992" coming from?

kalpitk added a comment.Jun 14 2019, 2:23 PM
In D1583#35782, @vlorentz wrote:

Where is the value "992" coming from?

bootstrap breakpoint
Below 992px means sm, xs devices.

In D1583#35776, @vlorentz wrote:

This needs a test (after D1577 lands)

Yeah sure

vlorentz added a comment.Jun 14 2019, 2:37 PM
In D1583#35783, @kalpitk wrote:

bootstrap breakpoint
Below 992px means sm, xs devices.

Oh, right!
Can we/should we import the constant from bootstrap instead of hardcoding it here?

kalpitk added a comment.EditedJun 14 2019, 2:55 PM
In D1583#35787, @vlorentz wrote:
In D1583#35783, @kalpitk wrote:

bootstrap breakpoint
Below 992px means sm, xs devices.

Oh, right!
Can we/should we import the constant from bootstrap instead of hardcoding it here?

We can't import these constants from bootstrap directly.

We can declare breakpoint constants globally for all the files.
If we want to have these constants for all the end to end tests ( where they will be mostly used), I think we can add the to index.js
Something like BP_XS, BP_SM, BP_MD, BP_LG may be globally declared constants

vlorentz added a comment.Jun 14 2019, 3:07 PM

I'd prefer them to be named BREAKPOINT_* instead of BP_*, but that sounds nice, thanks

kalpitk updated this revision to Diff 5255.Jun 14 2019, 7:48 PM
  • Add bootstrap breakpoints constants
swh-public-ci added a comment.Jun 14 2019, 7:54 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/491/ for more details.

Harbormaster completed remote builds in B6231: Diff 5255.Jun 14 2019, 7:54 PM
kalpitk updated this revision to Diff 5266.Jun 17 2019, 8:51 AM
  • Minor correction: md screen >=BREAKPOINT_MD rather than >BREAKPOINT_MD
kalpitk updated this revision to Diff 5267.Jun 17 2019, 8:56 AM
  • Update commit summary
swh-public-ci added a comment.Jun 17 2019, 8:58 AM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/492/ for more details.

Harbormaster completed remote builds in B6242: Diff 5266.Jun 17 2019, 8:58 AM
swh-public-ci added a comment.Jun 17 2019, 9:04 AM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/493/ for more details.

Harbormaster completed remote builds in B6243: Diff 5267.Jun 17 2019, 9:05 AM
anlambert requested changes to this revision.Jun 17 2019, 10:47 AM
anlambert added a subscriber: anlambert.

Just a small change to do and I will accept the diff.

swh/web/assets/src/bundles/webapp/webapp-utils.js
26–29

You should move this after line 6. Indeed, we backup the state of the sidebar in the localStorage to restore it when visiting the Software Heritage archive website again. Otherwise, I am OK with this fix.

This revision now requires changes to proceed.Jun 17 2019, 10:47 AM
anlambert mentioned this in D1567: Move Permalinks tabSlideout down in xs screens.Jun 17 2019, 11:02 AM
kalpitk added inline comments.Jun 17 2019, 11:30 AM
swh/web/assets/src/bundles/webapp/webapp-utils.js
26–29

But if the device is sm or xs we don't restore state from local storage. (Sidebar is always collapsed when you open any page irrespective of previous state)

anlambert accepted this revision.Jun 17 2019, 11:51 AM
anlambert added inline comments.
swh/web/assets/src/bundles/webapp/webapp-utils.js
26–29

Indeed, you are right. So this can be landed as is.

This revision now requires review to proceed.Jun 17 2019, 11:51 AM
vlorentz accepted this revision.Jun 17 2019, 3:13 PM
This revision is now accepted and ready to land.Jun 17 2019, 3:13 PM
Closed by commit rDWAPPS208d01b1fe9d: Fix sidebar not opening first time (authored by kalpitk). · Explain WhyJun 17 2019, 5:20 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
swh/
web/
assets/
src/
bundles/
browse/
4 lines
webapp/
10 lines
utils/
4 lines

Diff 5281

View Options

swh/web/assets/src/bundles/browse/browse-utils.js

Loading...
View Options

swh/web/assets/src/bundles/webapp/webapp-utils.js

Loading...
View Options

swh/web/assets/src/utils/constants.js

Loading...
About · Developer information · Legal