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.

Sign up for GitHub

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

Jump to bottom

Refactor two modals #861

Merged
merged 10 commits into from Oct 12, 2021
Merged

Refactor two modals #861

merged 10 commits into from Oct 12, 2021
+25 −22

Conversation

podviaznikov
Copy link
Contributor

@podviaznikov podviaznikov commented Oct 5, 2021 •
edited

  • one modal replaced with toast message
  • another one replaced with Dialog component

@vercel
Copy link

vercel bot commented Oct 5, 2021 •
edited

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/cal/calendso/9LEE95j2GwNLCXZBbcxgznpPrGdC
✅ Preview: https://calendso-git-feature-cal-392-refactor-modals-cal.vercel.app

@podviaznikov
Copy link
Contributor Author

new dialog for save profile
Profile___Cal_com

original
Profile___Cal_com

@podviaznikov
Copy link
Contributor Author

podviaznikov commented Oct 5, 2021 •
edited

original Cal branding dialog
Profile___Cal_com

new dialog
Profile___Cal_com

Not sure if we want to carry on original icons.

@vercel vercel bot temporarily deployed to Preview October 5, 2021 21:56 Inactive
@emrysal
Copy link
Contributor

emrysal commented Oct 5, 2021

@Jaibles tagging you in this due to design changes here. This is not a technical limitation Dialog vs. Model but a design choice.

@vercel vercel bot temporarily deployed to Preview October 7, 2021 08:03 Inactive
PeerRich
PeerRich approved these changes Oct 7, 2021
View reviewed changes
Copy link
Member

@PeerRich PeerRich left a comment •
edited

Choose a reason for hiding this comment

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

lets wait for @Jaibles before mergin

@PeerRich PeerRich self-requested a review October 7, 2021 16:34
@PeerRich
Copy link
Member

PeerRich commented Oct 7, 2021

new dialog for save profile Profile___Cal_com

original Profile___Cal_com

not a fan of the new save profile dialog @Jaibles do you have screens for this?

@PeerRich PeerRich added ❗️ changes requested Waiting for the author to implement fixes/suggestions 🚧 wip / in the making This is currently being worked on labels Oct 7, 2021
@baileypumfleet baileypumfleet removed the 🚧 wip / in the making This is currently being worked on label Oct 9, 2021
@PeerRich
Copy link
Member

we should continue to use the icons to make it visually more obvious what action happened

@vercel vercel bot temporarily deployed to Preview October 10, 2021 10:21 Inactive
@ciaranha
Copy link
Member

ciaranha commented Oct 10, 2021 •
edited

Quick notes:

  • For system feedback, we can just use toasts to confirm actions already done by the system
  • For destructive actions, we can use the dialog to confirm they want to do the action
  • The dialog (with the icon) is good for hitting the paid plan

I can provide designs to clarify these later if necessary!

@podviaznikov
Copy link
Contributor Author

oh, perfect. I will switch this to toasts!

@vercel vercel bot temporarily deployed to Preview October 11, 2021 18:43 Inactive
@vercel vercel bot temporarily deployed to Preview October 11, 2021 23:46 Inactive
@vercel vercel bot temporarily deployed to Preview October 11, 2021 23:56 Inactive
@vercel vercel bot temporarily deployed to Preview October 12, 2021 00:04 Inactive
@podviaznikov podviaznikov changed the title Refactor one modal into dialog. save profile Refactor two modals Oct 12, 2021
@podviaznikov
Copy link
Contributor Author

Updated dialog
Profile___Cal_com

@Jaibles does it look ok to you? We can tweak few classes. I also aligned text to the left and I think original is central alignment, but for some reason it didn't look that good to me. But happy to change.

Anton

@podviaznikov
Copy link
Contributor Author

Profile___Cal_com

And here is the toasts for the user profile update.

@vercel vercel bot temporarily deployed to Preview October 12, 2021 00:20 Inactive
emrysal
emrysal reviewed Oct 12, 2021
View reviewed changes
pages/settings/profile.tsx
<div className="sm:flex sm:items-start mb-4">
<div className="mt-3 sm:mt-0 sm:text-left">
<h3 className="font-cal text-lg leading-6 font-bold text-gray-900" id="modal-title">
This feature is only available in paid plan
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be "Pro plan". Not technically related to this PR

Copy link
Member

Choose a reason for hiding this comment

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

agree

emrysal
emrysal reviewed Oct 12, 2021
View reviewed changes
pages/settings/profile.tsx
<div className="mt-5 sm:mt-4 sm:flex sm:flex-row-reverse gap-x-2">
<DialogClose asChild>
<Button className="btn-wide btn-primary text-center" onClick={() => setModalOpen(false)}>
<span className="m-auto">Dismiss</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this span needed?

@vercel vercel bot temporarily deployed to Preview October 12, 2021 19:38 Inactive
PeerRich
PeerRich approved these changes Oct 12, 2021
View reviewed changes
pages/settings/profile.tsx
<div className="sm:flex sm:items-start mb-4">
<div className="mt-3 sm:mt-0 sm:text-left">
<h3 className="font-cal text-lg leading-6 font-bold text-gray-900" id="modal-title">
This feature is only available in paid plan
Copy link
Member

Choose a reason for hiding this comment

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

agree

@PeerRich
Copy link
Member

tested visually, works!

@PeerRich PeerRich merged commit 0871f5e into main Oct 12, 2021
@PeerRich PeerRich deleted the feature/cal-392-refactor-modals branch October 12, 2021 19:43
emrysal
emrysal reviewed Oct 12, 2021
View reviewed changes
pages/settings/profile.tsx
@@ -62,11 +64,18 @@ function HideBrandingInput(props: {
setModalOpen(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it is possible to wrap the <input> with <DialogTrigger asChild> with e.stopPropagation(); like:

(e) => (!e.currentTarget.checked || props.user.plan !== "FREE") && e.stopPropagation()

In no way something that needs doing; but one can be curious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emrysal emrysal emrysal left review comments

@PeerRich PeerRich PeerRich approved these changes

@baileypumfleet baileypumfleet baileypumfleet approved these changes

@ciaranha ciaranha Awaiting requested review from ciaranha

Assignees
No one assigned
Labels
❗️ changes requested Waiting for the author to implement fixes/suggestions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@podviaznikov @emrysal @PeerRich @ciaranha @baileypumfleet