Skip to content

Conversation

@HLeithner
Copy link
Member

Reduced duplicated code introduce by #27212 and added missing Fields.

Summary of Changes

Added a data render function to formfield
Refactor layouts to use preprocessed variable
Added missing fields

Testing Instructions

Code Review and check if Joomla forms till work especially the new fields.

Expected result

Works

Actual result

Works

Comment on lines 114 to 116
<?php echo $dataAttribute; ?>>
<div class="input-group">
<input <?php echo ArrayHelper::toString($inputAttributes); ?> readonly>
Copy link
Contributor

Choose a reason for hiding this comment

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

This was on <input> element before.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know but I think it should be on the joomla wc or I'm wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fancy select is on the input within the WC too. Hard to say which is better - there's an argument for adding support to both to be honest. Sometimes you want fill out the actual field value (i.e. the input), sometimes you want add properties to the WC for custom JS logic

Copy link
Member Author

Choose a reason for hiding this comment

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

@dgrammatiko any opinion on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes you want fill out the actual field value (i.e. the input), sometimes you want add properties to the WC for custom JS logic

This is very true. The reason you probably want to also fill a nested input in a custom element is to cover forms where the data is populated and the JS is disabled. If there is an input type=submit then the form will be submitted but if the input wasn't there then the backend will not get that particular input data which will lead to undesired behaviour. Edge case? Well, I don't know, I thought the core output should be consistent for all possible cases. Even if the core doesn't even use any such case. The Form Fields should be as close to metal as possible they're fundamental building blocks so their support should be solid. At least that was my approach, you can ditch it and dictate things, I mean Joomla is already doing this for the Form fields as they're still hardcoded to Bootstrap and Font Awesome which is definitely wrong if the aim is broad support...

In this case

<input <?php echo ArrayHelper::toString($inputAttributes); ?> <?php echo $dataAttribute; ?> readonly>

seems the change that could be merged without any testing

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I moved it to the input, but if js disabled you can't use it in the input anyway.

@wilsonge wilsonge merged commit 3f80758 into joomla:4.0-dev May 29, 2020
@wilsonge
Copy link
Contributor

Thanks!

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.

6 participants