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

Some doorkeeper proposals #126

Merged
merged 4 commits into from May 20, 2021
Merged

Conversation

gravitystorm
Copy link

Here's some proposed changes to your doorkeeper pull request. I think it's easier to open them as a PR against the branch on your repo, or at least I can't think of a better way.

They cover a few bootstrap / forms / i18n topics but there's nothing major here.

@tomhughes tomhughes force-pushed the doorkeeper branch 3 times, most recently from 520a575 to b96f386 Compare May 18, 2021 11:24
</tbody>
</table>
<% else %>
<p><%= t ".no_applications_html", :oauth2 => link_to(t(".oauth_2"), "https://oauth.net/2/") %></p>
Copy link
Owner

Choose a reason for hiding this comment

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

What's the reason for pulling out the link like that? It creates a potential lego issue in the translations and the main translation is already an HTML string anyway - is it just to stop translators changing the link target?

Copy link
Author

Choose a reason for hiding this comment

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

It was done this way just to match the corresponding no_apps_html from the OAuth (1) client applications page.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah ok. I just realised there are two versions of no_applications_html but only one of oauth_2 so I thought you might be sharing that but it seems not so I'm wondering if the other one works properly?

config/locales/en.yml Outdated Show resolved Hide resolved
flash:
applications:
create:
notice: Application Registered.
Copy link
Owner

Choose a reason for hiding this comment

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

Was there a reason for pulling this in from the upstream translations? I don't think we use it directly do we, though the doorkeeper controllers that we inherit from do.

Copy link
Author

Choose a reason for hiding this comment

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

It's part of the created vs registered thing - I wanted to keep the verb consistent between the page titles, action buttons and flash messages.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 90.631% when pulling b18c1bf on gravitystorm:doorkeeper into b96f386 on tomhughes:doorkeeper.

@tomhughes tomhughes merged commit 8e19f2c into tomhughes:doorkeeper May 20, 2021
@gravitystorm gravitystorm deleted the doorkeeper branch June 24, 2021 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants