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

Add site manager view and sidebar #1661

Merged
merged 49 commits into from
Aug 8, 2024
Merged

Add site manager view and sidebar #1661

merged 49 commits into from
Aug 8, 2024

Conversation

bgrgicak
Copy link
Collaborator

@bgrgicak bgrgicak commented Jul 31, 2024

Warning This PR contains a lot of TODOs because I didn't want it to get too large. We can decide if it's worth shipping as is or if we need to implement some of these missing features first.

Motivation for the change, related issues

Implements #1656

Browser storage in Playground supports having multiple sites by switching by adding a site-slug query string.
This is a powerful feature that's hard to discover.

As a first step in the Web app redesign project, this PR implements switching sites in browser storage.

Other site management features like adding and deleting sites will be added in future PRs.

Screenshot 2024-08-01 at 12 20 55
Screenshot 2024-08-01 at 12 21 08

Implementation details

The goal of this PR is to set the groundwork for the Web app redesign project by allowing users to switch between views.

The feature is only available while using browser storage.

The current view is now called site-view and the new management view is called site-manager.

In this iteration, switching is done by redirecting to a URL with a site-manager=true query string.
A future iteration will remove the need for reloads.

When in the site manager a list of sites is loaded from OPFS, clicking on a site (or preview) will redirect to that site.
This is a temporary implementation that will be removed once we add site storage.

Testing Instructions (or ideally a Blueprint)

@bgrgicak bgrgicak self-assigned this Jul 31, 2024
@bgrgicak bgrgicak marked this pull request as ready for review August 1, 2024 10:36
@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Aug 1, 2024

@adamziel @brandonpayton Is this feature as is already useful?

I would like to add a Add site button to the sidebar (in another PR) and merge these two PRs into production.
We can expand and fix TODOs from there.

This will allow us to ship early and iterate without the need for feature branches or even feature flags.

@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Aug 1, 2024

@adamziel mentioned we should avoid reloads when switching between views to ensure users don't lose progress.

We should also explore if there are any WordPress components that would be a good fit for the UI.

@adamziel
Copy link
Collaborator

adamziel commented Aug 2, 2024

Cc @jarekmorawski on components

@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Aug 6, 2024

I made the changes and it's now ready for a review.

Copy link
Member

@brandonpayton brandonpayton left a comment

Choose a reason for hiding this comment

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

Code-wise, this seems pretty nice and simple. ✨

I left a lot of style and UX-related feedback. Visually, this feels a little too rough to ship but maybe only slightly. If you disagree, please feel free to ship this, and we can iterate.

IMO, probably the most impactful changes would be:

  • Making the show/hide sidebar button have the same position regardless of whether the sidebar is open or closed.
  • Adjusting the toolbar and sidebar close/open transitions.
  • Considering leaving the embedded WP navigation textbox in place when the sidebar is open.

@@ -103,6 +102,13 @@ body.is-embedded .fake-window-wrapper {
height: 50px;
flex-direction: row;
align-items: center;
max-height: 100px;
overflow: hidden;
transition: max-height 0.14s ease-in-out;
Copy link
Member

Choose a reason for hiding this comment

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

These transitions feel a bit jarring to me. Subjectively, they feel too fast and also out-of sync. Specifically, the side bar transition in and the toolbar transition out feel out-of-sync with one another and jerky.

I think one of the reasons is timing, and the other may be that the sidebar appears immediately on the left while the contents slide into the sidebar from the right.

Screen.Recording.2024-08-06.at.2.54.19.PM.mov

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, this isn't great. I updated the animation to work similarly to the site-editor and synced the sidebar and toolbar animations. How does this look?

Screen.Recording.2024-08-07.at.11.19.39.mov

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The back animation could be a bit smoother, but I couldn't find a good way of doing it because the sidebar component is removed from DOM.

Copy link
Member

Choose a reason for hiding this comment

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

You are right, this isn't great. I updated the animation to work similarly to the site-editor and synced the sidebar and toolbar animations. How does this look?

This looks much better to me, and I no longer feel jarred when opening the sidebar.


.site-manager-sidebar-header {
display: flex;
padding: 16px 24px;
Copy link
Member

Choose a reason for hiding this comment

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

The padding around the header causes the "Close Sidebar" button to be positioned differently from the "Open Sidebar" button. Intuitively, my mind expects the button to act like an in-place toggle. In physical terms, it feels confusing like flipping a light switch on for a moment and finding that it has has moved when you reach to turn it off.

Screen.Recording.2024-08-06.at.3.00.23.PM.mov

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I Disabled logo clicks because this is how the design was initially created.

Making them overlap would require redesigning the sidebar.

I'm also considering disabling the view again to make it work as planned in the design.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will discuss this with @jarekmorawski today.

Copy link
Member

Choose a reason for hiding this comment

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

I Disabled logo clicks because this is how the design was initially created.

This seems like a fine solution, but AFAICT, there is now no way to close the sidebar, even if a site is clicked. Should the sidebar collapse once a site (even the current site) is clicked?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this might be the same concern you mentioned here.

}

.toolbar-hidden {
max-height: 0;
Copy link
Member

Choose a reason for hiding this comment

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

Why is the toolbar hidden when the sidebar is expanded? As a user, it feels like a distracting change and one that unnecessarily takes away the embedded WP navigation bar.

It seems practical to take away (or disable) the site configuration menus while the sidebar is opened to avoid conflicts between the two, but would it be possible to slide those out and let the embedded nav bar remain so users can continue seeing and setting the WP URL for the active session?

Leaving the toolbar in place might also reduce the number of moving parts mentioned in the "jarring" feedback above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted it to match the future design where the toolbar won't be visible.

Also the idea was for the site view to be disabled here but I changed it based on @adamziel feedback.

The current toolbar will eventually be removed, so I don't want to spend time adjusting it to work here for example we would need to hide the new menu button, adjust it to match visually, and add extra breakpoints to keep responsive support.

@jarekmorawski what do you think?

Copy link
Member

@brandonpayton brandonpayton 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 we are in a place where this is good enough to ship and iterate on. Thank you, @bgrgicak! It's great to see this progress on the redesign.

@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Aug 7, 2024

Thanks for the review @brandonpayton!

I got some feedback from @jarekmorawski today that I plan to address tomorrow and ship it.

The most important thing is that clicking on the preview while the sidebar is open will hide the sidebar instead of allowing interactions with Playground while in the manager view.
That part of the UI is intended as a preview and doesn't even exist on mobile. Its main purpose is to see the site you selected.
If we would allow for interactions the UI would quickly fall apart. For example, this is how it looks with the site editor.

Screenshot 2024-08-07 at 13 39 48

All other changes will be minor style adjustments.

@brandonpayton
Copy link
Member

I got some feedback from @jarekmorawski today that I plan to address tomorrow and ship it.

Sounds good! I think what you said about the preview makes sense.

Btw, I merged the latest from trunk into this PR to resolve a conflict. After that, I was planning to merge as-is, so I'm glad you commented before that happened. :)

Long doc comments are hard to read and it's easy to write long comments.

We need an automated way of checking for the comment length.

This PR adds
[eslint-plugin-comment-length](https://www.npmjs.com/package/eslint-plugin-comment-length?activeTab=readme)
to implement comment max-len `eslint` rules.

The errors aren't fixed to avoid creating issues with this PR, but they
will show up as warnings, so we can fix them in the future.

- CI
@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Aug 8, 2024

I addressed all the feedback and this is now ready to be merged.

@bgrgicak bgrgicak merged commit 0f11a6f into trunk Aug 8, 2024
4 of 5 checks passed
@bgrgicak bgrgicak deleted the add/website-view-switcher branch August 8, 2024 09:27
@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Aug 8, 2024

Offline e2e test were failing everything else works as expected, so I merged the PR.

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

Successfully merging this pull request may close these issues.

4 participants