[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

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

[mui-bot]

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

Details of bundle changes

Generated by 🚫 dangerJS against b9311f5

[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?

👍

[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]

You only need to edit drawers.md and this file will be translated later.

[siriwatknp]

Can you add a note that the demo only works on the mobile viewport? Also, I try to play this feature in iOS chrome but it does not work (Swipe on the left seems to navigate to the previous page instead).

[tech-meppem]

I believe that's because disableDiscovery is disabled by default on iOS, so it wont work.
I can set it to always be enabled if you think that's better, but that might impact expected navigation on the page itself.

[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.

[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: #30414

[tech-meppem]

I cannot continue to work on the unit tests, due to drawer discovery being completely broken in React 18:
#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.