[SwipeableDrawer] Add `appearOffset` prop

[tech-meppem]

Adds a prop that changes how much the drawer moves when the user "discovers" it, by touching in the swipe-area.

Closes #30762

• [x] I have followed (at least) the PR section of the contributing guide.

[mui-bot]

	Messages
:book:	Netlify deploy preview: https://deploy-preview-30763--material-ui.netlify.app/

Details of bundle changes

Generated by :no_entry_sign: dangerJS against b9311f500784eb31d6d3f0618dc88bc902d6dcb7

[hbjORbj]

@tech-meppem

Hi. Thank you for opening this PR.

Can you add a demo describing the use of the prop you are proposing (discoveryAmount)?

You can add to docs/pages/src/pages/components/drawers.

[hbjORbj]

Hi. Thanks for adding the demo.

In case you didn't know, you can find it here

[michaldudak]

"discoveryAmount" does not sound intuitive. IMO something like "discoveryZoneSize" would describe the intent of this prop better.

[tech-meppem]

"discoveryAmount" does not sound intuitive. IMO something like "discoveryZoneSize" would describe the intent of this prop better.

Personally I think that confuses it with the swipe area width, as it's the amount the drawer moves on discovery, and not the size of the area you can click to discover the drawer. Maybe something like "discoveryMoveAmount"?

[michaldudak]

The word "amount" doesn't feel right to me in this context. Perhaps "discoveryDistance", then?

[hbjORbj]

discoveryZoneSize sounds much clearer to me than discoveryAmount or discoveryDistance.

@siriwatknp What do you think?

[tech-meppem]

I think discoveryZoneSize makes it sound like you're changing the area where you can click to discover the drawer, which is not what this prop is doing. I think it's specifically the word "zone" that's easily confusing there.

Edit: Perhaps discoveryOffset?

[michaldudak]

Perhaps discoveryOffset?

:+1:

[siriwatknp]

Perhaps discoveryOffset?

👍

This is from my thought when I first saw this PR. discovery is very confusing, I don't understand what I would get. However, when I read the description I can understand what this prop is about from this sentence

You can make the drawer appear slightly when the user touches the screen near where the drawer is located.

I think the word appear is the key, so I think appearanceOffset is better than discoveryOffset.

[siriwatknp]

👋 The migration PR has been merged.

Please follow these steps to make sure that the contents are all updated. (Sorry for the inconvenience)

1. pull latest master from upstream to your branch
2. if your PR has changes on the *.md or demo files, you are likely to encounter conflict because all of them have been moved to the new folder. 2.1 revert the change on those markdown files you did 2.2 pull latest master from upstream (you should not see conflict) 2.3 make the changes again at docs/data/material/*
3. run yarn docs:api
   • you might see the changes in docs/pages/material/api/* if your PR touches some of the components
   • it is okay if there is no changes

If you are struggle with the steps above, feel free to tag @siriwatknp

[hbjORbj]

Okay, I think we have these options so far: discoveryOffset, appearOffset and let me add one more option, which is displayOffset.

[siriwatknp]

Okay, I think we have these options so far: discoveryOffset, appearOffset and let me add one more option, which is displayOffset.

I am good with appearOffset or displayOffset.

[hbjORbj]

In fact, we are missing tests. Would you be willing to add them? @tech-meppem

[tech-meppem]

In fact, we are missing tests. Would you be willing to add them? @tech-meppem

I can try, although I'm not sure how they are done here. I can't see anything about adding them on the CONTRIBUTING doc (as in, examples of where / how). If there's an overview somewhere of how, and what is expected of the test, I can see what I can do.

[hbjORbj]

This would be a good reference: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.test.js :)

[tech-meppem]

This would be a good reference: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.test.js :)

I'm looking at that, and while it's somewhat helpful, I'm not really sure how to apply the test for this prop. I have made a test that applies the prop, but unsure of how to actually test the prop is working.

I imagine you would need to do something like: (say [x,y] is near bottom of the screen where drawer is).

• Test position [x, y+20] that the drawer is not there.
• fire touchStart at [x,y].
• Test position [x, y+20] that the drawer is now there.
• fire touchEnd
• Test position [x, y+20] that the drawer is not there.

I can't figure out how you would test for the amount the drawer has moved between touchStart and touchEnd, as all other tests look for specific property values, or the callCount of open / closed, instead of the position of elements on the page. I can't find an example of how that would be done.

[tech-meppem]

@hbjORbj What system / package is being used for the tests? Where can I look up for docs / help on how to solve the problem I listed above?

[hbjORbj]

@tech-meppem

Hey, sorry for late reply.

Test position [x, y+20] that the drawer is not there. fire touchStart at [x,y]. Test position [x, y+20] that the drawer is now there. fire touchEnd Test position [x, y+20] that the drawer is not there.

This seems right. You can test whether the component has translated using getBoundingClientRect() since it provides top, bottom, left and right.

Here is an example:

  it.only('prop: appearOffset', function test() {
    // this allows you to test only on browser
    // this is needed since we want to use `getBoundingClientRect`
    if (/jsdom/.test(window.navigator.userAgent)) {
      this.skip();
    }
    render(
        <SwipeableDrawer ... />,
      );
    const drawer = screen.getByTestId('drawer');
    const initialRect = drawer.getBoundingClientRect();
    fireEvent.touchStart(drawer, {
      touches: [new Touch({ identifier: 0, target: drawer, pageX: 250, clientY: 0 })],
    });
    const rectAfterTouchStart = drawer.getBoundingClientRect();
    ...compare `initialRect` and `rectAfterTouchStart`
    fireEvent.touchEnd(drawer, {
      touches: [new Touch({ identifier: 0, target: drawer, pageX: 250, clientY: 0 })],
    });
    const rectAfterTouchEnd = drawer.getBoundingClientRect();
    ...compare `initialRect` and `rectAfterTouchEnd`
    };

You can run yarn test:karma in order to test this with browser.

In fact, for this prop, a visual test would be more effective, meaning that your demo is already sufficient for test already. This is because we use argos to detect any changes in demos when new merge requests open in the future. However, adding a unit test won't hurt either.

[hbjORbj]

@tech-meppem Hi. Would you like to finish this up? :)

[tech-meppem]

@tech-meppem Hi. Would you like to finish this up? :)

I was struggling to get the tests to run for a while, as they would just crash / error for me (even without any edits). I think I got it working now, so I'll see if I can finish it up soon.

[tech-meppem]

@hbjORbj After trying many tests of many varieties, I cannot get it to work. I don't know if it's because of the testing environment, or something, but the behaviour seems inconsistent with how it is in the browser. Or, the render hasn't updated properly in order to actually test it.

Here is the code:

  describe('prop: appearOffset', () => {
    it('should make drawer move by the amount set', function test() {
      // this allows you to test only on browser
      // this is needed since we want to use `getBoundingClientRect`
      if (/jsdom/.test(window.navigator.userAgent)) {
        this.skip();
      }
      const appearOffsetMoveAmount = 40;
      const handleOpen = spy();
      render(
        <SwipeableDrawer
          anchor="left"
          open={false}
          onOpen={handleOpen}
          onClose={() => {}}
          keepMounted
          appearOffset={appearOffsetMoveAmount}
          transitionDuration={0}
          SwipeAreaProps={{ 'data-testid': 'swipearea' }}
          PaperProps={{ 'data-testid': 'drawer' }}
        >
          <div style={{ width: 100, height: 100 }} /* data-testid='drawer' */>SwipeableDrawer</div>
        </SwipeableDrawer>
      );
      const swipeArea = document.querySelector('[class*=PrivateSwipeArea-root]');
      const drawer = screen.getByTestId('drawer');

      const initialRect = drawer.getBoundingClientRect();
      fireEvent.touchStart(swipeArea, {
        touches: [new Touch({ identifier: 0, target: swipeArea, pageX: 15, clientY: 0 })],
      });

      const rectAfterTouchStart = drawer.getBoundingClientRect();
      console.log('rects', initialRect.x, rectAfterTouchStart.x, drawer, swipeArea, handleOpen.callCount)
      expect(rectAfterTouchStart.x).to.equal(initialRect.x + appearOffsetMoveAmount);

      // fireEvent.touchEnd(swipeArea, {
      //   touches: [new Touch({ identifier: 0, target: swipeArea, pageX: 25, clientY: 0 })],
      // });
      // const rectAfterTouchEnd = drawer.getBoundingClientRect();
      // expect(rectAfterTouchEnd.x).to.equal(initialRect.x);
    });
  });

And here is the error output of the console.log:

LOG: 'rects', -100, 0, <div class="MuiPaper-root MuiPaper-elevation MuiPaper-elevation16 MuiDrawer-paper MuiDrawer-paperAnchorLeft emotion-client-render-4t3x6l-MuiPaper-root-MuiDrawer-paper" data-testid="drawer" tabindex="-1" style="pointer-events: none; transform: none; transition: transform 0ms cubic-bezier(0, 0, 0.2, 1) 0ms;"><div style="width: 100px; height: 100px;">SwipeableDrawer</div></div>, <div class="PrivateSwipeArea-root 
emotion-client-render-n2n0ek" data-testid="swipearea" style="width: 20px;"></div>, 0
Chrome Headless 98.0.4695.0 (Windows 10) <SwipeableDrawer /> prop: appearOffset should make drawer move by the amount set FAILED
        AssertionError: expected +0 to equal -60
            at Context.test (webpack-internal:///./packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.test.js:17:134792)

LOG: 'rects', -100, 0, shows the x value of the Paper is switching from -100 to 0, and not from -100 to -60 as you'd expect (and testing on codesandbox via the demos, and in my own work project, -100 to -60 is what I was getting). It should only go to 0 after the drawer is fully opened, and not in it's "discovery state".

The fact that transform: none is set, rather than transform: translateX(-100px); is something that happens only when the drawer is fully opened, and not while being dragged open.

Testing on a codesandbox with demos (and in my own work project) shows that the transform: translate(...) is there even if the drawer is fully open, and only after letting go of touch (touchEnd event) does the transform: none get set.

Removing the transitionDuration={0} means that the drawer doesn't move at all before the code runs, as there is now a transition, so it wouldn't be an instant change.

Changing the element with data-testid='drawer' has no noticeable effect, and re-getting the elements post-touchStart in case of a remount shows there is no difference in the element instances.

I really don't know what the problem is here. Is it possible to make these tests async, and then await delays? I'm not familiar with this specific library for testing.

p.s. found this on line 234 of SwipeableDrawer.test.js: acnhor={params.anchor}

Edit: I believe this behaviour is due to discovery being broken in React 18: https://github.com/mui/material-ui/issues/30414

[tech-meppem]

I cannot continue to work on the unit tests, due to drawer discovery being completely broken in React 18: https://github.com/mui/material-ui/issues/30414

Due to that, the tests for it are just malfunctioning, and as such, do not work. I will do the tests once this is fixed.

[mnajdova]

It should be possible to add tests now. @tech-meppem I know it's been a while, are you still interested in continuing the effort?

[tech-meppem]

It should be possible to add tests now. @tech-meppem I know it's been a while, are you still interested in continuing the effort?

I am, and I will try to. I just haven't got much time, and I'm not familiar with the test library. I will try and finish the tests soon.

[ZeeshanTamboli]

I am, and I will try to. I just haven't got much time, and I'm not familiar with the test library. I will try and finish the tests soon.

It's been a long time since any activity, so I am closing this PR. @tech-meppem If you are willing to continue working, feel free to re-open the PR and push the remaining changes. You can at least try adding tests, and the team will help you if you need any help.