Page MenuHomeSoftware Heritage

Fix sidebar not opening first time
ClosedPublic

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

Details

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

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

Where is the value "992" coming from?

Where is the value "992" coming from?

bootstrap breakpoint
Below 992px means sm, xs devices.

This needs a test (after D1577 lands)

Yeah sure

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

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

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
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
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
27–30

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
kalpitk added inline comments.Jun 17 2019, 11:30 AM
swh/web/assets/src/bundles/webapp/webapp-utils.js
27–30

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
27–30

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
This revision was automatically updated to reflect the committed changes.