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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure only bordering areas of app are draggable #82

Merged
merged 3 commits into from
May 7, 2024

Conversation

SiobhyB
Copy link

@SiobhyB SiobhyB commented May 3, 2024

Related to 6805-gh-Automattic/dotcom-forge#issuecomment-2089930508

Proposed Changes

  • Following the feedback at 6805-gh-Automattic/dotcom-forge#issuecomment-2089930508, tweaks have been made so that the space between the app's sidebar the and its main content area is no longer draggable.
  • Instead, it should only be possible to click and drag the bordering areas of the app. The left sidebar remains draggable, also.

Testing Instructions

Verify only bordering areas of the app are draggable

  • The space between the sidebar and the app's content should no longer be draggable.
  • It should be possible to click and drag any bordering area of the app, including the main sidebar to the left.

Ensure no changes to layout

As this PR tweaks some of the CSS used in the app's main content area, we should also verify that there are no unwanted changes to the app's general layout:

Before After

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@SiobhyB SiobhyB added the [Type] Enhancement Improvement upon an existing feature label May 3, 2024
@SiobhyB SiobhyB self-assigned this May 3, 2024
@SiobhyB SiobhyB marked this pull request as ready for review May 3, 2024 04:19
@SiobhyB SiobhyB requested review from a team and wojtekn May 3, 2024 04:20
Copy link
Contributor

@derekblank derekblank left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

I observed that:

  • The targeted bordering areas of the app were draggable
  • Layout issues were not introduced (including checking RTL layouts)

Copy link
Contributor

@katinthehatsite katinthehatsite left a comment

Choose a reason for hiding this comment

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

The changes look good to me. I confirm that I saw the region around app borders was draggable along with the sidebar 👍 Nice work!

@wojtekn
Copy link
Contributor

wojtekn commented May 6, 2024

@SiobhyB I still can't make it work. I marked areas that are draggable for me, and used arrow to show the one that doesn't work:

Screenshot 2024-05-06 at 10 09 17

@katinthehatsite
Copy link
Contributor

Actually, I re-tested it and confirm that I see the same thing as Wojtek for the left side of the sidebar. All the other areas seem to work as expected for me.

@wojtekn
Copy link
Contributor

wojtekn commented May 6, 2024

While testing this branch, I also noticed that scrolling sites list down makes the top left area not draggable:

Screenshot 2024-05-06 at 12 05 03 Screenshot 2024-05-06 at 12 05 12

This happens on the trunk, too, so it looks like it is unrelated to changes from this branch.

As per the feedback here, the left border of the app wasn't draggable when multiple sites were listed in the sidebar: #82 (comment)

This commit addresses that.
@SiobhyB
Copy link
Author

SiobhyB commented May 7, 2024

@wojtekn, thanks for the screenshot! I had been testing with only one local site, which is why I wasn't experiencing the issue with the left border not being draggable, I've been able to reproduce what you're seeing after adding a long list of sites.

I've addressed this in 0db7235. Let me know if that resolves what you're seeing!

@fluiddot, would you be able to verify this doesn't result in any unwanted layout shifts in Windows, too? I'd be grateful for the testing as this latest change involves different margins for Windows. 🙇‍♀️

@SiobhyB
Copy link
Author

SiobhyB commented May 7, 2024

While testing this branch, I also noticed that scrolling sites list down makes the top left area not draggable

@wojtekn, I wasn't able to reproduce that issue with the latest changes. Here's what I'm seeing:

Screen.Recording.2024-05-07.at.09.58.03.mov

Are you still able to reproduce?

@wojtekn
Copy link
Contributor

wojtekn commented May 7, 2024

thanks for the screenshot! I had been testing with only one local site, which is why I wasn't experiencing the issue with the left border not being draggable, I've been able to reproduce what you're seeing after adding a long list of sites.

Thanks, it works well now - I can use left border to drag, too.

I wasn't able to reproduce that issue with the latest changes. Here's what I'm seeing:

I can reproduce it in the trunk, but this branch fixes that.

@fluiddot
Copy link
Contributor

fluiddot commented May 7, 2024

@fluiddot, would you be able to verify this doesn't result in any unwanted layout shifts in Windows, too? I'd be grateful for the testing as this latest change involves different margins for Windows. 🙇‍♀️

Sure, I'm going to test it and report back.

@fluiddot
Copy link
Contributor

fluiddot commented May 7, 2024

@fluiddot, would you be able to verify this doesn't result in any unwanted layout shifts in Windows, too? I'd be grateful for the testing as this latest change involves different margins for Windows. 🙇‍♀️

Sure, I'm going to test it and report back.

@SiobhyB I've tested the PR on Windows and haven't noticed any changes in the layout. Additionally, I tested the draggable regions and confirmed it works as described in the PR 🎊 .

@SiobhyB
Copy link
Author

SiobhyB commented May 7, 2024

@wojtekn, @fluiddot, awesome, thanks so much! I'll go ahead to merge 🚀

@SiobhyB SiobhyB merged commit c6a0b37 into trunk May 7, 2024
10 checks passed
@SiobhyB SiobhyB deleted the fix/tweak-draggable-regions branch May 7, 2024 12:17
Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

👋🏻 @SiobhyB I noticed these changes slightly crop the focus outline style for the sites navigation. Would you be willing to explore if this is straightforward to rectify please? 🙇🏻

Before After
image image

@SiobhyB
Copy link
Author

SiobhyB commented May 8, 2024

@dcalhoun, thank you for catching that! I've opened #102 to address it. 🙇‍♀️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement Improvement upon an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants