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

Fields extra attributes checks #691

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

enrico-sorcinelli
Copy link
Contributor

This PR aims to solve following problems:

  • prevent overriding attributes causing wrong behaviours. For example, adding a checkbox field like:
new Fieldmanager_Checkbox( 'Check test', array(
   'name' => 'test_checkbox',
   'attributes' => array(
      'checked' => true
   )
) );

cause the element to be always checked independently of previously saved value. Similarly, setting id attribute cause some problem on labels handling and so on.
This is because extra HTML attributes are printed out before id and checked.

  • avoid duplication of attributes by setting attributes like class, type, name, id, value, checked and some data-*. In HTML5 it's not allowed to have two or more identical attributes on the same tag. What happens in practice is that browsers ignore the latter attribute. Well, future browsers might do otherwise (that is, the parser is allowed to stop when it encounters an error).
    This was done by adding an attributes black list to get_element_attributes().

  • allow to add classes to class attribute of field itself (not only to his container).
    This can be useful in some situation if you want to reuse of CSS classes without adding the same rules in specific selectors. In practice:

new Fieldmanager_Textfield( 'Input test', array(
   'name' => 'test_input',
   'attributes' => array(
      'class' => 'my-class'
   )
) );

will append my-class to element class.

PS: I added unit tests only to checkbox field (I think is sufficiently, but I can add to others too).

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.

1 participant