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

Chrome: Adding a confirm utility to replace window.confirm calls #960

Closed
wants to merge 2 commits into from

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented May 31, 2017

Testing instructions

Try to toggle the "private" visibility to see the dialog.

@youknowriad youknowriad self-assigned this May 31, 2017
@youknowriad youknowriad changed the title WIP/ Add/accept dialog WIP: Add/accept dialog May 31, 2017
@youknowriad youknowriad changed the title WIP: Add/accept dialog Chrome: Adding a confirm utility to replace window.confirm calls Jun 8, 2017
@youknowriad youknowriad requested a review from aduth June 8, 2017 09:46
@youknowriad youknowriad added the General Interface Parts of the UI which don't fall neatly under other labels. label Jun 8, 2017
@jasmussen
Copy link
Contributor

Pushed a little polish. I think we might want to do more polish, but this is great for now!

@aduth
Copy link
Member

aduth commented Jun 8, 2017

Out of curiosity: Why do we want this?

@youknowriad
Copy link
Contributor Author

Out of curiosity: Why do we want this?

I don't know, It just seems a better UI to me :). I expect us to need this later for deletions and stuff like that?

@aduth
Copy link
Member

aduth commented Jun 8, 2017

I expect us to need this later for deletions and stuff like that?

window.confirm would work just as well?

Maybe not as pretty 😛 But also consistent / expected.

Can't always use the styled dialog anyways, e.g. beforeunload (though from what I hear, browsers are starting to prevent this "leave confirmation" for window.confirm as well).

@jasmussen
Copy link
Contributor

Can't always use the styled dialog anyways, e.g. beforeunload (though from what I hear, browsers are starting to prevent this "leave confirmation" for window.confirm as well).

Great question to be asking! I'm actually fine with using the stock confirm box. Can we translate it?

@aduth
Copy link
Member

aduth commented Jun 8, 2017

Can we translate it?

Yep

@jasmussen
Copy link
Contributor

I think the styled version looks decent, but since we have no urgent need for it, and it's conceivable it could come with various accessibility issues we'd want to address, perhaps we should go with window.confirm for now, and then revisit this PR if and when it becomes necessary.

@youknowriad
Copy link
Contributor Author

Ok Closing for now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants