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 confirm prompt to control panel #253

Merged
merged 4 commits into from
May 11, 2022
Merged

Conversation

neil-ptr
Copy link
Contributor

@neil-ptr neil-ptr commented May 6, 2022

Closes: #38

Description

Add prompt to ControlPanel component warning user that redeploying account will reset account states instead of using the native browser confirm method


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@vercel
Copy link

vercel bot commented May 6, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
flow-playground ✅ Ready (Inspect) Visit Preview May 10, 2022 at 4:22PM (UTC)

@neil-ptr
Copy link
Contributor Author

neil-ptr commented May 6, 2022

Screen Shot 2022-05-06 at 3 45 04 PM

@srinjoyc
Copy link

srinjoyc commented May 6, 2022

Looking good! One small note:

Would consider changing up the color scheme a bit to something like this for UX:

image

^ IE. A neutral outline on the cancel, and a red for destructive action ('Confirm'). The text could be red and the warning icon could be yellow.

Reference

@vercel vercel bot temporarily deployed to Preview May 6, 2022 23:02 Inactive
@neil-ptr
Copy link
Contributor Author

neil-ptr commented May 6, 2022

Screen Shot 2022-05-06 at 5 01 34 PM

Updated the UI based on @srinjoyc 's recommendations

@srinjoyc srinjoyc self-requested a review May 6, 2022 23:37
@MaxStalker
Copy link
Contributor

MaxStalker commented May 9, 2022

Screen Shot 2022-05-06 at 5 01 34 PM

Updated the UI based on @srinjoyc 's recommendations

Well done, @neilZon !
Personally, I would have put that text on top of the buttons (not to the left) - to make that panel slightly more compact.
But it's not a requirement 😅

switch (true) {
case isOk && showPrompt:
statusIcon = (
<FaExclamationTriangle style={{ color: theme.colors.warning }} />
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to set color of this one here:

export const ControlContainer = styled.div<ControlContainerProps>`
display: flex;
align-items: center;
justify-content: space-between;
color: ${({ isOk, progress }) => {
switch (true) {
case progress:
return '#a2a2a2';
case isOk:
return '#2bb169';
default:
return '#EE431E';
}
}};
`;

@neil-ptr
Copy link
Contributor Author

neil-ptr commented May 9, 2022

Screen Shot 2022-05-09 at 11 12 59 AM

@MaxStalker how does this look?

@vercel vercel bot temporarily deployed to Preview May 9, 2022 17:25 Inactive
@MaxStalker
Copy link
Contributor

MaxStalker commented May 9, 2022

Again, personally I would do something like this (maybe even adjust line height on text block)
Screenshot from 2022-05-09 20-35-23

Screenshot from 2022-05-09 20-35-01

But (I think) you should tag @srinjoyc or @kerrywei - they are calling these shots :)

@neil-ptr
Copy link
Contributor Author

neil-ptr commented May 9, 2022

@kerrywei @srinjoyc Thoughts on the above pics?

@kerrywei
Copy link

kerrywei commented May 9, 2022

@kerrywei @srinjoyc Thoughts on the above pics?

@neilZon what's your thought on the pros and cons of these two designs?

@kerrywei
Copy link

kerrywei commented May 9, 2022

I have a big +1 to not use green text and use red instead, to make the warning stand out

@neil-ptr
Copy link
Contributor Author

neil-ptr commented May 9, 2022

@kerrywei @srinjoyc Thoughts on the above pics?

@neilZon what's your thought on the pros and cons of these two designs?

I think @MaxStalker 's recommendation is better since we are taking up less space with the prompt message so I'll probably go that way

@neil-ptr
Copy link
Contributor Author

Screen Shot 2022-05-10 at 10 19 22 AM

Screen Shot 2022-05-10 at 10 19 27 AM

Heres what it should look like now @MaxStalker @kerrywei @srinjoyc

@vercel vercel bot temporarily deployed to Preview May 10, 2022 16:22 Inactive
@neil-ptr neil-ptr merged commit a4250b1 into staging May 11, 2022
@bthaile bthaile deleted the neilzon/38-redeploy-popup branch May 16, 2023 22:01
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.

Clicking redeploy should open a popup
4 participants