-
Notifications
You must be signed in to change notification settings - Fork 18
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
Studio: Improve offline mode for Sync tab #666
base: trunk
Are you sure you want to change the base?
Conversation
Since we do not have the exact design for this, I have some open questions that I felt as comments on the PR. Feel free to make any suggestions! |
onSelectSite={ setSelectedSiteId } | ||
/> | ||
{ isOffline && ( | ||
<div className="absolute inset-0 bg-white/80 z-10 flex items-center justify-center"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this needs more or less transparency. I tried with something like bg-white/90
and it seems like it is not transparent enough to me. Happy to adjust this though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. 👍
{ __( 'Push' ) } | ||
</Button> | ||
</div> | ||
<Tooltip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added one tooltip for these two buttons as they are too close too each other and are too small and making two tooltips seemed too crowded and excessive. Happy to change this though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Good call @katinthehatsite
…-offline-mode-for-sync-tab
</Button> | ||
</Tooltip> | ||
</> | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to extract these two buttons in the common component as we are using them in two places and the code is quite repetitive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me @katinthehatsite. Thanks!
I spotted an unrelated issue with the alignment of the offline icon in the header bar. Logged here: #668
Thanks for catching this, Matt. Turns out this is a regression introduced in #648. I will remove the |
Related issues
Closes https://github.com/Automattic/dotcom-forge/issues/9808
Proposed Changes
This PR handles offline mode for the
Sync
tab.On the modal: if the connection goes out when the user has the modal open:
On the Sync tab itself:
Testing Instructions
STUDIO_SITE_SYNC=true npm start
Sync
tab that has some sites connectedCreate a new site
andConnect site
are disabledPull
orPush
buttons, they are disabled and you can see a tooltip with informationConnect site
to open a modalPre-merge Checklist