Skip to content

feat: Add "Outdated" tooltip and "Update version" button in the Workspaces page #2322

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

Merged
merged 7 commits into from
Jun 15, 2022

Conversation

BrunoQuaresma
Copy link
Collaborator

Demo:
https://user-images.githubusercontent.com/3165839/173636455-33c83eb3-2525-46cd-a198-6d9ec785eb81.mov

Important things:

  • Each workspace/row is a machine so each workspace can execute your actions and handle your own state.
  • We added polling in the workspaces machine, so the list is updated automatically.
  • We are using an optimistic update to set the workspace into the "Queued" state. I think this is way better than having a spinner inside of the tooltip and tracking it down in the component.
  • To update the workspaces machines I had to code some "complex" logic because:
    • If we just recreate all the machines, we would lose the workspace state. Eg. Let's say one workspace is updating the version and after polling the new workspace data we recreate it. It will lose all the previous states because it will be a new instance.
    • To avoid that, I had to verify if new items were created and spawning, update the ones with new data and stop and remove the actors that are not present anymore.
    • You can see that in the action updateWorkspaceRefs
  • I'm not adding tests right now because it will take some time and I have to jump into other priority website tasks. Knowing that, is ok we open an exception and merge this if the current tests are ok? I promise to add the tests when I finish the website priority stuff. 🤞

Closes #2001

@BrunoQuaresma BrunoQuaresma self-assigned this Jun 14, 2022
@BrunoQuaresma BrunoQuaresma requested a review from a team as a code owner June 14, 2022 17:19
const helpTooltipContext = useContext(HelpTooltipContext)

if (!helpTooltipContext) {
throw new Error("This hook should be used in side of the HelpTooltipContext.")
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@@ -76,6 +84,78 @@ const WorkspaceHelpTooltip: React.FC = () => {
)
}

const OutdatedHelpTooltip: React.FC<{ onUpdateVersion: () => void }> = ({ onUpdateVersion }) => {
Copy link
Member

Choose a reason for hiding this comment

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

I think this file is getting really big.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is but, for now, I think it is ok. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

We can leave it for now. I want to talk about directory structure at the next FE Variety - maybe we can look at pulling stuff apart then.


export const workspacesMachine = createMachine(
{
tsTypes: {} as import("./workspacesXService.typegen").Typegen0,
tsTypes: {} as import("./workspacesXService.typegen").Typegen1,
Copy link
Member

Choose a reason for hiding this comment

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

What's this Typegen1 vs Typegen0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is auto-generated by XState types: https://xstate.js.org/docs/guides/typescript.html#typegen

export const HelpTooltip: React.FC<HelpTooltipProps> = ({ children, open, size = "medium" }) => {
const styles = useStyles({ size })
const [anchorEl, setAnchorEl] = useState<HTMLButtonElement | null>(null)
open = open ?? Boolean(anchorEl)
const id = open ? "help-popover" : undefined

const onClose = () => {
setAnchorEl(null)
}
Copy link
Member

Choose a reason for hiding this comment

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

Seems like when I click to escape the tooltip, regardless of where I click, I am automatically routed to the workspace page. Sometimes I just want to close the tooltip without leaving the list. Could we suppress navigation on close?

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 think it is because you are clicking on a workspace row when you click outside of the tooltip.

Copy link
Member

@Kira-Pilot Kira-Pilot Jun 15, 2022

Choose a reason for hiding this comment

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

It does it even if I click somewhere else on the page. I think it is using the initial click into the tooltip to navigate.

Copy link
Member

@Kira-Pilot Kira-Pilot left a comment

Choose a reason for hiding this comment

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

Pulled this down and it looks and works great!

@BrunoQuaresma
Copy link
Collaborator Author

Chromatic deploys are failing, I reviewed the storybooks and they look good. I send a support message to support@chromatic.com.

@BrunoQuaresma BrunoQuaresma enabled auto-merge (squash) June 15, 2022 13:38
@BrunoQuaresma BrunoQuaresma merged commit b225953 into main Jun 15, 2022
@BrunoQuaresma BrunoQuaresma deleted the bq/add-outdated-tooltip branch June 15, 2022 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance: Version column 'outdated' status to allow update in place
2 participants