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

CRM-21599: Added missing structure in templates #11457

Merged
merged 2 commits into from
Dec 28, 2017

Conversation

mukeshcompucorp
Copy link
Contributor

@mukeshcompucorp mukeshcompucorp commented Dec 26, 2017

Overview

This PR

  • Adds missing HTML structure at templates/CRM/Admin/Page/LocationType.tpl
  • Adds missing label Event Name of event at templates/CRM/Event/Form/SearchEvent.tpl

Before

before

After

after

Technical Details

Tabing and spacings are improved


</div>
<div style="height: auto; vertical-align: bottom">{$form.eventsByDates.html}</div>
<td>
<label>{ts}Event Nanme{/ts}</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo - should be "Event Name". Can you get this from $form.title.label?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattwire Fixed at 92164de

@@ -72,4 +73,5 @@
{crmButton q="action=add&reset=1" id="newLocationType" icon="plus-circle"}{ts}Add Option{/ts}{/crmButton}
{crmButton p="civicrm/admin" q="reset=1" class="cancel" icon="times"}{ts}Done{/ts}{/crmButton}
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this file related to field name change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pradpnayak This is not just for adding label. PR is to make the structure proper and this file adds missing wrapper around the content like other files.

Copy link
Contributor

Choose a reason for hiding this comment

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

:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I understand :) as approved then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes APPROVED.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pradpnayak can you do that from above Review changes then? for others to know the status

@pradpnayak
Copy link
Contributor

I am not sure if its a great idea to have label for the text field since there is enough description about the usage of the field. I will this to core team to decide.

I have tested this locally and it works as expected. However, I am not confident of having 'templates/CRM/Admin/Page/LocationType.tpl' file included in this PR. Is it some how related to the fix? And also can you squash the commits?

@mukeshcompucorp
Copy link
Contributor Author

@pradpnayak
to your question

I am not sure if its a great idea to have label for the text field since there is enough description about the usage of the field. I will this to core team to decide.

Description should always depict more about field but, it is the label that ultimately tells people what the field is for.
That's why WCAG standards says to have a label for each input,
so, label -> field name/title
description -> more about what you can do to the field

@monishdeb
Copy link
Member

monishdeb commented Dec 27, 2017

I am not opposed to these change but like @pradpnayak said we already had an inline description for this field. But still, if we need to show the label then we also need to made such changes to other Search forms like:

  1. Manage Group screen: screen shot 2017-12-27 at 8 24 04 pm
  2. Manage Contribution page: screen shot 2017-12-27 at 8 23 22 pm

@mukeshcompucorp would you like to discuss the PR on our dev channel https://chat.civicrm.org/civicrm/channels/dev ?

@monishdeb
Copy link
Member

Tested the PR and works for me. Doesn't bring any unintended change for the additional fixes made in localtionType.tpl. But next time please bring the indentation fixes or any NFC changes in separate PR.

Manage Event page after applying this patch:
screen shot 2017-12-28 at 3 42 40 pm

@monishdeb monishdeb merged commit 7db7ba6 into civicrm:master Dec 28, 2017
sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018
CRM-21599: Added missing structure in templates
@mlutfy mlutfy added this to the 4.7.31 milestone Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants