Skip to content

[4.0] Allow jForm to support data attribute#27212

Merged
HLeithner merged 27 commits intojoomla:4.0-devfrom
vaivk369:4.0-dev
May 28, 2020
Merged

[4.0] Allow jForm to support data attribute#27212
HLeithner merged 27 commits intojoomla:4.0-devfrom
vaivk369:4.0-dev

Conversation

@vaivk369
Copy link
Copy Markdown

@vaivk369 vaivk369 commented Dec 5, 2019

Co-Authored-By: Nicola Galgano <optimus4joomla@gmail.com>
@alikon
Copy link
Copy Markdown
Contributor

alikon commented Dec 5, 2019

is this a work in progress ?
as example this one has not been added from #16141
https://github.com/joomla/joomla-cms/blob/4.0-dev/layouts/joomla/form/field/color/advanced.php

@Quy Quy changed the title Allow jForm to support data attribute 'Allow jForm to support data attribute Dec 5, 2019
@Quy Quy changed the title 'Allow jForm to support data attribute [4.0] Allow jForm to support data attribute Dec 5, 2019
@dgrammatiko
Copy link
Copy Markdown
Contributor

@twsvaishali please extend this for aria-*

@vaivk369
Copy link
Copy Markdown
Author

vaivk369 commented Dec 6, 2019

is this a work in progress ?
as example this one has not been added from #16141
https://github.com/joomla/joomla-cms/blob/4.0-dev/layouts/joomla/form/field/color/advanced.php

Have added remaining fields in latest commits. Please check.

@vaivk369
Copy link
Copy Markdown
Author

vaivk369 commented Dec 6, 2019

@twsvaishali please extend this for aria-*

Can we start new PR for this?

@alikon
Copy link
Copy Markdown
Contributor

alikon commented Dec 6, 2019

Have added remaining fields in latest commits. Please check.

seems fine now thanks

yes maybe another PR for the aria-*

@vaivk369 vaivk369 requested a review from Quy December 6, 2019 11:13
@dgrammatiko
Copy link
Copy Markdown
Contributor

@alikon @twsvaishali for the aria-* all is needed is a var ariaAttributes
and adding

				// Check for aria attribute
 				if (strpos($name, "aria-") === 0 && array_key_exists($name, $this->ariaAttributes))
 				{
 					return $this->ariaAttributes[$name];
 				}

in the main class and some minor adjustments in the layouts. Of course it can be done in another PR to make things easier for testing. I just had to mention that, since a11y is always an afterthought...

@dgrammatiko
Copy link
Copy Markdown
Contributor

@twsvaishali since you're redoing this maybe we could change the layouts logic to:

if (!empty($dataAttributes)) {
	// Data attributes - data-*
	$dataAttribute = '';

	foreach ($dataAttributes as $key => $attrValue)
	{
		$dataAttribute .= ' ' . $key . '="' . htmlspecialchars($attrValue, ENT_COMPAT, 'UTF-8') . '"';
	}
}

In essence the loop will execute only if there are any data-* attributes defined.
Micro optimisation? Maybe, maybe not, I haven't benchmarked the code but this seems reasonable...

@vaivk369
Copy link
Copy Markdown
Author

vaivk369 commented Dec 6, 2019

@dgrammatiko Yes. Will start another PR for the aria-* once this is tested and merged. Also I am actually not sure if all fields will have area attribute.

@vaivk369 vaivk369 requested a review from Quy December 7, 2019 12:27
@richard67
Copy link
Copy Markdown
Member

I have tested this item ✅ successfully on d46baf1

Works so far.

Maybe some fields are missing because they did not exist (yet) when this PR was made.

It needs someone who checks for completeness and if necessary makes a new PR.

Same with the idea to do the same for "aria-" attributes, this should be done in another PR, too.


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

@Quy
Copy link
Copy Markdown
Contributor

Quy commented May 27, 2020

I have tested this item ✅ successfully on d46baf1


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

@Quy
Copy link
Copy Markdown
Contributor

Quy commented May 27, 2020

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 27, 2020
@richard67
Copy link
Copy Markdown
Member

@wilsonge @Quy What about the following field layouts? They are not touched by this PR. Shall they be done, too?

  • checkboxes.php
  • color/slider.php
  • contenthistory.php
  • radio/buttons.php
  • radio/switcher.php
  • tag.php

@HLeithner HLeithner merged commit 15674ee into joomla:4.0-dev May 28, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 28, 2020
@HLeithner
Copy link
Copy Markdown
Member

Thanks all!

@HLeithner HLeithner added this to the Joomla 4.0 milestone May 28, 2020
sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020
Co-authored-by: Nicola Galgano <optimus4joomla@gmail.com>
Co-authored-by: Quy <quy@fluxbb.org>
Co-authored-by: George Wilson <georgejameswilson@googlemail.com>
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
Co-authored-by: Harald Leithner <leithner@itronic.at>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants