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

Changed 'Submit' text on buttons with something more meaningful #3749

Merged
merged 5 commits into from Mar 9, 2018

Conversation

@shubham76
Copy link
Contributor

@shubham76 shubham76 commented Mar 7, 2018

This is regarding #3735.
I have changed 'Submit' text on buttons with 'Save', 'Add' etc. Also made some other typo changes.
@stsewd & @davidfischer please have a look.

@@ -72,7 +72,7 @@ <h4>{% trans "Opting out" %}</h4>
{% endblocktrans %}

<form method="get" action="{% url "gold_subscription" %}">
<input type="submit" value="{% trans "I can contribute financially" %}" />
<input type="submit" value="{% trans "I can contribute financially." %}" />
Copy link
Member

@stsewd stsewd Mar 7, 2018

Choose a reason for hiding this comment

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

Not sure if it's ok to include periods within a button.

Loading

Copy link
Contributor Author

@shubham76 shubham76 Mar 7, 2018

Choose a reason for hiding this comment

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

It's more like an entire sentence. That's why included that. Should I change it back the way it was?

Loading

Copy link
Member

@stsewd stsewd Mar 7, 2018

Choose a reason for hiding this comment

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

I don't have the arguments or the knowledge for choosing one or the other, was just a question p: But in general the PR should only solve the original issue, if you consider something else is needed is better to open a new issue, so your PR isn't blocked by discussions like this :)

Loading

Copy link
Contributor Author

@shubham76 shubham76 Mar 8, 2018

Choose a reason for hiding this comment

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

Yes. You are right. I am kind of new here. Sorry! It won't happen again! 👍

Loading

Copy link
Member

@stsewd stsewd Mar 8, 2018

Choose a reason for hiding this comment

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

Nothing to worried about. Thanks for your interest on contributing.

Loading

Copy link
Contributor Author

@shubham76 shubham76 Mar 9, 2018

Choose a reason for hiding this comment

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

It's a great learning for me. Thanks for your constant help :)

Loading

{% comment %}Translators: The 'or' here is in between 'Submit' and 'delete'.{% endcomment %}
{% trans "or" %}
<a href="{% url "projects_delete" project.slug %}">{% trans "delete" %}</a>
<a href="{% url "projects_delete" project.slug %}">{% trans "delete project" %}</a>
Copy link
Member

@RichardLitt RichardLitt Mar 7, 2018

Choose a reason for hiding this comment

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

Why isn't this capitalized, too?

Loading

Copy link
Contributor Author

@shubham76 shubham76 Mar 8, 2018

Choose a reason for hiding this comment

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

I'll update it with changes. Thanks. :)

Loading

@@ -19,10 +19,10 @@
</p>
{{ form.as_p }}
<p>
<input style="display: inline;" type="submit" value="{% trans "Submit" %}">
<input style="display: inline;" type="submit" value="{% trans "Save" %}">
{% comment %}Translators: The 'or' here is in between 'Submit' and 'delete'.{% endcomment %}
Copy link
Member

@stsewd stsewd Mar 8, 2018

Choose a reason for hiding this comment

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

We need to update this comment :)

Loading

Copy link
Contributor Author

@shubham76 shubham76 Mar 8, 2018

Choose a reason for hiding this comment

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

Oh! Yes. My bad.

Loading

Copy link
Contributor

@agjohnson agjohnson left a comment

This is a great addition! It always bothers me to see a Submit button :)

We don't use title case in our UI, but especially on buttons -- or are at least we're moving away and standardizing on sentence case. I noted a few instances, but there are a few more in this PR. Could you revert those?

Loading

@@ -66,7 +66,7 @@ <h3>{% trans "Build a version" %}</h3>
<option value="{{ version.slug }}">{{ version.slug }}</option>
{% endfor %}
</select>
<input type="submit" value="{% trans "Build" %}">
<input type="submit" value="{% trans "Build Project" %}">
Copy link
Contributor

@agjohnson agjohnson Mar 9, 2018

Choose a reason for hiding this comment

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

As this is a build version form, perhaps this should be Build version instead

Loading

Copy link
Contributor Author

@shubham76 shubham76 Mar 9, 2018

Choose a reason for hiding this comment

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

Oh! Yes. You are right. I'll change it right away!

Loading

@@ -18,6 +18,6 @@
action="{% url 'projects_integrations_create' project_slug=project.slug %}">
{% csrf_token %}
{{ form.as_p }}
<input type="submit" value="{% trans "Add integration" %}">
<input type="submit" value="{% trans "Add Integration" %}">
Copy link
Contributor

@agjohnson agjohnson Mar 9, 2018

Choose a reason for hiding this comment

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

We don't use title casing on our buttons, or at least shouldn't

Loading

Copy link
Contributor Author

@shubham76 shubham76 Mar 9, 2018

Choose a reason for hiding this comment

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

Oh! Okay. I'll revert those changes. 👍

Loading

@@ -17,7 +17,7 @@
<li>
<a class="button"
href="{% url 'projects_integrations_create' project_slug=project.slug %}">
{% trans "Add integration" %}
{% trans "Add Integration" %}
Copy link
Contributor

@agjohnson agjohnson Mar 9, 2018

Choose a reason for hiding this comment

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

No title case here too

Loading

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Mar 9, 2018

This looks good to me, thanks for your contribution! 🎉

Loading

@agjohnson agjohnson merged commit d0c6abb into readthedocs:master Mar 9, 2018
1 check passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants