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

[4.0] Subfields - refactor for accessibility etc #26614

Merged
merged 18 commits into from
Oct 19, 2019

Conversation

brianteeman
Copy link
Contributor

Fix the layout issue as reported with #26602 and numerous other fixes

summary

  • set column widths
  • Consistency rename all instances of "sub field" to "sub fields"
  • Apply style guide rule for labels
  • change the table class and add table id
  • replace links with button and remove useless tabindex
  • kill the description for the render value field but keep it in the code as a tip (it doesn't need to be repeated on every row)
  • Several code style changes
  • remove icon-white class from buttons - it was not required and resulted in the icons not being centered

Notes

  • It looks to me from the code that I should be able to have repeatable fieldgroups but I couldnt see how
  • Really not sure why the fields are in a for each loop when there are only ever the same two. Maybe I am missing something

@Quy
Copy link
Contributor

Quy commented Oct 17, 2019

kill the description for the render value field but keep it in the code as a tip (it doesn't need to be repeated on every row)

Still repeated. See under Global Configuration > Users > Email Domain Options

26614

Would it be better to display it once in the table header instead in a tooltip?

@brianteeman
Copy link
Contributor Author

Ah I didnt know it was being used by core anywhere. I will review that later and update this PR. Is core using this anywhere else?

Would it be better to display it once in the table header instead in a tooltip?

Yes it would

@brianteeman
Copy link
Contributor Author

Now that I see the email options page uses this I can see even more issues with the original code - how the heck was this tested and merged

@Quy
Copy link
Contributor

Quy commented Oct 17, 2019

Missing language string under Global Configuration > Users > Email Domain Options.

<caption id="captionTable" class="sr-only">
				PLG_FIELDS_SUBFIELDS_TABLE_CAPTION			</caption>

Is core using this anywhere else?

Under Plugins > System - Redirect, but layout is slightly different. I don't know if there are others.

@AndySDH
Copy link
Contributor

AndySDH commented Oct 18, 2019

Now that I see the email options page uses this I can see even more issues with the original code - how the heck was this tested and merged

See this: #24711

This was required to be merged by the PR creator for funcionallity wise as over a year of work went into it.

Layout/appearance wise it does indeed still need work.

@brianteeman
Copy link
Contributor Author

Layout/appearance wise it does indeed still need work.

Not just that as seen from the long list :)

@jduerscheid
Copy link

@brianteeman: Please add some testinstructions


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26614.

@uglyeoin
Copy link
Contributor

I have tested this item ✅ successfully on b8403d7

All seems to work well.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26614.

@Quy
Copy link
Contributor

Quy commented Oct 19, 2019

I have tested this item ✅ successfully on b8403d7

The repeating description can be addressed in a separate PR.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26614.

@Quy
Copy link
Contributor

Quy commented Oct 19, 2019

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26614.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 19, 2019
@wilsonge wilsonge merged commit 1444750 into joomla:4.0-dev Oct 19, 2019
@wilsonge
Copy link
Contributor

Thanks!

@joomla-cms-bot joomla-cms-bot removed RTC This Pull Request is Ready To Commit labels Oct 19, 2019
@wilsonge wilsonge added this to the Joomla 4.0 milestone Oct 19, 2019
@brianteeman
Copy link
Contributor Author

Thanks

@Quy can you open an issue for the repeated description please

@brianteeman brianteeman deleted the subfields branch October 19, 2019 21:06
@AndySDH
Copy link
Contributor

AndySDH commented May 31, 2020

The repeated description in the Email Domain Options page is still an issue in J4 Beta

@wilsonge
Copy link
Contributor

@AndySDH can you please open a fresh issue so it can be tracked

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants