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

added a confirm message to the remove recording method #38

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

Conversation

simonjcarr
Copy link

Added a confirm dialog message before a recording is deleted.

@netlify
Copy link

netlify bot commented Aug 2, 2020

‼️ Deploy request for camati rejected.
Learn more about Netlify's sensitive variable policy

🔨 Explore the source changes: eaa98aa

@simonjcarr simonjcarr marked this pull request as ready for review August 2, 2020 12:51
Copy link
Owner

@ackzell ackzell left a comment

Choose a reason for hiding this comment

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

Thank you!
I added a comment.

components/RecordingsList.vue Outdated Show resolved Hide resolved
@simonjcarr
Copy link
Author

simonjcarr commented Aug 2, 2020 via email

@simonjcarr simonjcarr requested a review from ackzell August 3, 2020 16:34
@ackzell
Copy link
Owner

ackzell commented Aug 11, 2020

Thanks a lot for your contribution @simonjcarr!

I have some feedback still 😬

In the image you can see the dialog grows as wide as there is space available in the window. Can we make that not the case and constrain the width?

As for the buttons, I was thinking to align them to the right and change the colors to be:

  • Cancel: Secondary
  • Delete: Primary

If possible, maybe some space around them to not be too close to the edges of the dialog. (I glanced on the docs and I think that is the default spacing, so that would be okay to remain the same).

Pasted_Image_8_10_20__8_39_PM

@simonjcarr
Copy link
Author

simonjcarr commented Aug 12, 2020 via email

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.

2 participants