Add a "Close" button to the OAuth dialog, rename "Done" button elsewhere

[charlescapps]

In this previous PR #3939, @Minasokoni added a "Done" button to the auth dialog, however this was only included for the non-Oauth authentications. This PR adds a "Close" button for OAuth authentication as well, and renames the button for non-OAuth to "Close".

Per discussion in this feature request (#4211), we agreed to rename the done button to "Close".

Description

The OAuth dialog still didn't have a "Close" button.

This was confusing some of our users, because it's rather odd that you have to click the x to close the dialog and keep your authentication. This PR fixes this so that the "Close" button is also present for OAuth dialogs.

Motivation and Context

Fully fixes #3537; fixes #4211 as well.

How Has This Been Tested?

I built my changes with npm run build and then used the static resources from the dist folder for one of our Swagger APIs that has an oauth2 type authentication. See screenshots below. The "Close" button works to close the dialog.

Screenshots (if appropriate):

Before this PR, OAuth dialog:

After this PR, OAuth dialog:

Checklist

My PR contains...

• No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
• Dependency changes (any modification to dependencies in package.json)
• Bug fixes (non-breaking change which fixes an issue)
• Improvements (misc. changes to existing features)
• Features (non-breaking change which adds functionality)

My changes...

• are breaking changes to a public API (config options, System API, major UI change, etc).
• are breaking changes to a private API (Redux, component props, utility functions, etc.).
• are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
• are not breaking changes.

Documentation

• My changes do not require a change to the project documentation.
• My changes require a change to the project documentation.
• If yes to above: I have updated the documentation accordingly.

Automated tests

• My changes can not or do not need to be tested.
• My changes can and should be tested by unit and/or integration tests.
• If yes to above: I have added tests to cover my changes.
• If yes to above: I have taken care to cover edge cases in my tests.
• All new and existing tests passed.

We could add a Selenium test for clicking the "Done" button but it's not super important.

[charlescapps]

I'll rebase against master and rebuild dist later to resolve merge conflicts. Busy Saturday right now!

[heldersepu]

You might want to look into #4211
it seems that Done button is confusing some...

I personally do not think there should be "Done" or "Cancel" button
...and nothing odd about the x on the top right corner to close the dialog, that has been standard in computers for a few decades.

[charlescapps]

Sorry @heldersepu, I respectfully disagree.

Users find it confusing they have to click the x at the top-right because most dialogs have an OK or some equivalent. Closing something out gives the appearance that you are canceling it. There's no positive UI element for continuing. Users will accidentally click the logout button because both logout and the x are negative actions, and the logout button is the only big button they are presented with.

I agree that advertisements and so on typically only have an x because you're closing them out, but this isn't an advert. There should be some positive button for continuing, like the Update comment button immediately below where I'm typing! Yes, even GitHub has positive buttons for every action.

I wouldn't mind making this configurable, but I think for now we should just make this consistent. It's currently in a zombie state where OAuth is inconsistent with non-OAuth.

[charlescapps]

Another example -- the brand new version of JIRA has a Save and Cancel button, with no x in the top right, when you edit most anything. We could change the terminology to Save, but I don't see how a positive button is old-fashioned.

[heldersepu]

@charlescapps you might want to leave a comment on #4211
...start a healthy debate (all the users and engineers at that company might disagree)

[charlescapps]

Ha. Well, we should just make this configurable then. I don't think any one solution is going to make everyone happy.

Yeah, I'm fine with a compromise, I think we can fix both of our issues. I'm fine with calling it Cancel initially as in that feature request, then changing the text to Done after you have an authorization. That would also work without requiring a config option.

[charlescapps]

Thanks for pointing out that existing feature request @heldersepu . @hoereth said that renaming it to Close is a good compromise, so I updated this pull request with that wording.

[heldersepu]

I don't think you need to commit the files under /dist I've never done that...

[charlescapps]

ok @heldersepu whoops, I see that now in contributing.md. I just reverted the changes in dist.

[hoereth]

@charlescapps : thank you very much! :)

[charlescapps]

No worries, glad there was an easy compromise. Let me know if there's anything else needed to merge this, @heldersepu . Thanks!

[charlescapps]

@heldersepu is there anything else before this can be merged? @hoereth is happy with this as a fix to #4211. Sorry if my initial reply to your comment was a tad much, I was just feeling frustrated because we have to maintain a lot of custom branches of various open source projects. I appreciate you pointing out the related PR so we could find a compromise. I'm also always up for making a feature configurable and off by default when needed. Thanks!

[heldersepu]

Looks good to me, this change is very simple...
But I do not do merges, that will be @shockey

[shockey]

thanks @charlescapps!