Skip to content
This repository was archived by the owner on Jun 26, 2018. It is now read-only.

toggle js-location-listing class at layout breakpoint#631

Open
clairesunstudio wants to merge 6 commits intodevfrom
DP-5525_disable-scroll-to-map-mobile
Open

toggle js-location-listing class at layout breakpoint#631
clairesunstudio wants to merge 6 commits intodevfrom
DP-5525_disable-scroll-to-map-mobile

Conversation

@clairesunstudio
Copy link
Copy Markdown
Contributor

@clairesunstudio clairesunstudio commented Oct 16, 2017

Description

Disable the scroll up to map on mobile on location listing- this feels very jarring, and also does not add any additional info.

Related Issue / Ticket

Steps to Test

  1. resize window to 910px breakpoint
  2. for window size > 910px, not change
  3. for window size < 911px, click/hover on any location listing, check the following:
  • disable auto scroll to the map
  • disable active state
  • disable map update

Screenshots

location-listing

Additional Notes:

Help needed:

when resize down to window size < 911px, map doesn't recenter.

Impacted Areas in Application

  • N/A

@todo

  • N/A

Today I learned...

Copy link
Copy Markdown
Contributor

@hackajesse hackajesse left a comment

Choose a reason for hiding this comment

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

Passes external review, but needs a changelog @clairesunstudio. Moving to WIP so you can do that. Once you do, move directly to Internal Review for @isaacchansky.

@isaacchansky
Copy link
Copy Markdown
Contributor

Sorry for the super delayed review on this. I think a simpler way to do this might be to just bind the click/mouseover/etc events once, and check the screen-width upon interaction to either do nothing or continue as normal. Rough code would look like:

$el.on('click', function(e) {

  if($(window).width()+15 > 910) {
    // do stuff
  }

});

This would avoid the binding/unbinding issues, and simply bind once.

Copy link
Copy Markdown
Contributor

@isaacchansky isaacchansky left a comment

Choose a reason for hiding this comment

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

See previous comment, happy to chat about this on Monday!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants