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

228 save disabled popup #273

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

228 save disabled popup #273

wants to merge 2 commits into from

Conversation

lache-melvin
Copy link
Collaborator

Closes #228

πŸ‘©πŸ»β€πŸ’» What does this PR do?

If a notification has a valid configuration, but the status is set to disabled, show this popup when they hit save:
Screenshot 2023-11-27 at 5 00 39 PM

πŸ§ͺ How has/should this change been tested?

  • Create a new notification
  • Fill out required fields, keep the Enabled toggle off
  • Hit save, this modal should appear
  • Select Cancel
  • Enable the notification, hit save again - see that saving works right away
  • Disable the notification again, and hit save again
  • You should get the modal again, this time select Ok
  • Your disabled config is saved

πŸ’Œ Any notes for the reviewer?

  • I think the wording could be clearer? Of what happens on cancel and ok?
  • Wanted a popup with Enable & Save and Save anyway or something like that, but was limited by the confirmation modal to Cancel and Ok (if we think the former options are really worth it we could create a new modal, but would take longer...
  • Feels a bit clunky that when you hit ok, the notification is saved, but you stay on the same page. For some reason, now that there is a modal, I'm expecting to be redirected back to the list of notification. Does it feel the same to you? Could redirect after... or maybe a success snack popup would help?

Copy link

Bundle size difference

Comparing this PR to main

Old size New size Diff
2.14 MB 2.14 MB 585 B (0.03%)

@jmbrunskill jmbrunskill self-requested a review November 28, 2023 01:06
Copy link
Collaborator

@jmbrunskill jmbrunskill left a comment

Choose a reason for hiding this comment

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

I think this needs to be Enable and Save or similar otherwise it's just annoying in my testing.
I tried to hack up a tool tip, that just reminded the user that they might want to enable but for some reason the Tooltips aren't showing on Loading Button???

Lets' just leave it for now. Not urgent!

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.

Reminder to enable a configuration
2 participants