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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Scrollable]adjust scroll to bottom for lower resolution issues #4218

Merged
merged 1 commit into from
May 26, 2021

Conversation

rox163
Copy link
Contributor

@rox163 rox163 commented May 18, 2021

WHY are these changes introduced?

When launching our draft orders feature, a merchant reported an issue with the scrolalble area on th e product resource picker. The 'fetch more' action didnt seem to trigger when they scrolled to the bottom of a paginated list. The support specialist in touch with the merchant seemed to have narrowed it down to be reproducible only on lower resolution screens.
See original issue here


Broken scroll (it shouldve loaded 4-5 more more products after Sleek Granite shoes)
Broken scroll

WHAT is this pull request doing?

After some pairing and irl chats with @dleroux we decided to add a small buffer to the height detection for the scrolledToBottom action on a Scrollable component. Unfortunately none of our dev machines are able to go down to a lower resolution so i cant reproduce the original problem locally.


Gif of playground testing with a scrollable modal
Gif of playground testing with a scrollable modal

Browserstack test on Windows Edge (scroll in general was pretty slow and jumpy in Edge through browserstack fyi):


Gif of browserstack with low res and zoom WITH the fix
Gif of browserstack with low res and zoom WITH the fix

How to 馃帺

馃枼 Local development instructions
馃棐 General tophatting guidelines
馃搫 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React, {useCallback, useRef, useState} from 'react';

import {Button, Modal, Page, Stack, TextContainer} from '../src';

export function Playground() {
  const [active, setActive] = useState(true);
  const node = useRef(null);


  const toggleModal = useCallback(() => setActive((active) => !active), []);

  const activator = <Button onClick={toggleModal}>Open</Button>;

  return (
    <Page title="Playground">
      <div style={{height: '500px'}}>
        <Modal
          activator={activator}
          open={active}
          onClose={toggleModal}
          primaryAction={{
            content: 'Close',
            onAction: toggleModal,
          }}
          onScrolledToBottom={() => console.log('scrolled to bottom')}
        >
          <Modal.Section>
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <br />
            <Stack vertical>
              <Stack.Item>
                <TextContainer>
                  <p>
                    You can share this discount link with your customers via
                    email or social media. Your discount will be automatically
                    applied at checkout.
                  </p>
                </TextContainer>
              </Stack.Item>
            </Stack>
          </Modal.Section>
        </Modal>
      </div>
    </Page>
  );
}

馃帺 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @鈥奌YPD, @鈥妋irualves, @鈥妔arahill, or @鈥妑y5n to update the Polaris UI kit

@rox163 rox163 self-assigned this May 18, 2021
@rox163 rox163 changed the title [WIP]adjust scroll to bottom for lower resolution issues [WIP][Scrollable]adjust scroll to bottom for lower resolution issues May 18, 2021
@rox163 rox163 force-pushed the fix-scroll-event-lower-resolution branch from 142c757 to 6624508 Compare May 18, 2021 18:30
@github-actions
Copy link
Contributor

github-actions bot commented May 18, 2021

size-limit report

Path Size
cjs 141.86 KB (+0.01% 馃敽)
esm 95.43 KB (+0.01% 馃敽)
esnext 138.57 KB (+0.01% 馃敽)
css 33.68 KB (0%)

@rox163 rox163 force-pushed the fix-scroll-event-lower-resolution branch from 6624508 to 0b2b969 Compare May 19, 2021 14:02
@rox163 rox163 requested a review from dleroux May 19, 2021 14:31
@rox163 rox163 marked this pull request as ready for review May 20, 2021 20:02
@rox163 rox163 changed the title [WIP][Scrollable]adjust scroll to bottom for lower resolution issues [Scrollable]adjust scroll to bottom for lower resolution issues May 20, 2021
@rox163 rox163 requested a review from kyledurand May 20, 2021 20:02
Copy link
Contributor

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

I think I follow what's happening here. Can we somehow 馃帺 this fix on a low res device in browser stack?

src/components/Scrollable/Scrollable.tsx Outdated Show resolved Hide resolved
@rox163
Copy link
Contributor Author

rox163 commented May 20, 2021

oh browser stack!! ill give that a shot

@rox163
Copy link
Contributor Author

rox163 commented May 20, 2021

I just requested access to our shopify browserstack account. so will wait and see.

@rox163
Copy link
Contributor Author

rox163 commented May 21, 2021

finally reproduced it in browserstack! Res of 1280x800 and with a 125% zoom set in browser as well!

@rox163 rox163 force-pushed the fix-scroll-event-lower-resolution branch from 0b2b969 to e1f4a85 Compare May 21, 2021 21:21
@rox163 rox163 requested a review from kyledurand May 21, 2021 21:21
Copy link
Contributor

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Thanks for 馃帺 ing!

Copy link
Contributor

@dleroux dleroux left a comment

Choose a reason for hiding this comment

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

Awesome that you could reproduce finally. 馃檱 馃帀

@rox163 rox163 force-pushed the fix-scroll-event-lower-resolution branch from e1f4a85 to eaca14b Compare May 26, 2021 17:03
@rox163 rox163 merged commit 9081df8 into main May 26, 2021
@rox163 rox163 deleted the fix-scroll-event-lower-resolution branch May 26, 2021 18:26
@DerekSerre
Copy link

Adding on to @rox163 's comment. I can replicate as well by setting the browsers zoom to anything other than 100%. Replicated using a range between 90% - 125% of browsers zoom in chrome on my macbooks native resolution. Also confirmed it happens in Safari on any zoom level, but not firefox. Tested on my own store.

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

Successfully merging this pull request may close these issues.

None yet

4 participants