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

make sidekick opening interactive with storage #375

Merged
merged 4 commits into from
Feb 27, 2023

Conversation

mrval5
Copy link
Contributor

@mrval5 mrval5 commented Feb 23, 2023

What does this do?

  • store the state of sidekick if it's open or closed.
  • in case no value in the storage, we open the sidekick and we store the value.

Why are we making this change?

How do I test this?

Key decisions and Risk Assessment:

Things to consider:

  1. How will this affect security?
  2. How will this affect performance?
  3. Does this change any APIs?

@mrval5 mrval5 requested a review from a team February 23, 2023 15:52
@AdamDotCom AdamDotCom self-requested a review February 23, 2023 17:20
Copy link
Contributor

@dalefukami dalefukami left a comment

Choose a reason for hiding this comment

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

I may be interpreting the story incorrectly but should we also be making sure the correct tab is selected? Maybe you already have a follow up PR coming.

src/components/sidekick/index.tsx Outdated Show resolved Hide resolved
src/store/layout/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@AdamDotCom AdamDotCom left a comment

Choose a reason for hiding this comment

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

I noticed a sidekick animation flash.

To reproduce:

  • Connect, minimize the sidekick
  • Disconnect
  • Connect, observe that the sidekick opens and closes
sidekick-flash.webm

I wonder if you'll have to reset the canStartAnimation state when a user disconnects?

src/components/sidekick/index.test.tsx Outdated Show resolved Hide resolved
src/components/sidekick/index.test.tsx Outdated Show resolved Hide resolved
src/components/sidekick/index.test.tsx Outdated Show resolved Hide resolved
src/components/sidekick/index.tsx Outdated Show resolved Hide resolved
src/store/layout/index.ts Outdated Show resolved Hide resolved
src/components/sidekick/index.test.tsx Outdated Show resolved Hide resolved
@mrval5
Copy link
Contributor Author

mrval5 commented Feb 24, 2023

I may be interpreting the story incorrectly but should we also be making sure the correct tab is selected? Maybe you already have a follow up PR coming.

the story has nothing about the tab. I can create another task - PR for the tab.

@dalefukami
Copy link
Contributor

the story has nothing about the tab

"When I open messenger, it should open in the same view as i last had it"

I would consider the tab I had selected to be part of the "same view"

@dalefukami
Copy link
Contributor

Sorry, I should have been more clear. That was my interpretation. Do it whichever way you want. ;)

@mrval5
Copy link
Contributor Author

mrval5 commented Feb 24, 2023

the story has nothing about the tab

"When I open messenger, it should open in the same view as i last had it"

I would consider the tab I had selected to be part of the "same view"

not sure if you remember that we cloned the story (id 2, 214) to split the sidekick and messenger state to 2 stories
@AdamDotCom explained during the daily that this sprint we will work on the sidekick.

I'm a bit confused here. maybe better to work on both stories in the same time.

@dalefukami
Copy link
Contributor

Ah, yeah, when we were talking about splitting the story my understanding was that we were splitting off remember the state of the chat window (which chat was open and if it was minimized/fullscreen, etc) and the sidekick. My interpretation was that the tabs in the sidekick are part of the sidekick. It's obviously a separate PR from this, but new story, same story, doesn't really matter. Just chat with Adam and make a choice. He's representing the user.

@mrval5 mrval5 merged commit e8cc9db into main Feb 27, 2023
@mrval5 mrval5 deleted the sidekick-store-open-value branch February 27, 2023 15:46
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.

None yet

3 participants