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

Updated Standard Fields index #356

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

Conversation

marcorensch
Copy link
Contributor

@marcorensch marcorensch commented Dec 27, 2024

User description

Added a section that lists optional options for all field types


PR Type

Documentation


Description

  • Added comprehensive documentation section for common options available to all form field types
  • Listed and described 13 standard options that are inherited from the base FormField class
  • Each option includes its default value and functional description
  • Documented important form field behaviors like autocomplete, spellcheck, hidden states, and accessibility features
  • Added deprecation notice for 'validate' option with reference to proper validation documentation

Changes walkthrough 📝

Relevant files
Documentation
index.md
Documentation of Common Form Field Options                             

docs/general-concepts/forms-fields/standard-fields/index.md

  • Added new section documenting common options available across all form
    field types
  • Listed 13 standard options including autocomplete, spellcheck, hidden,
    disabled, etc.
  • Included default values and descriptions for each option
  • Added note about deprecated 'validate' option with reference to
    client-side validation
  • +17/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Added a section that lists optional options for all field types
    Copy link
    Contributor

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Documentation Accuracy

    The autocomplete option description appears incomplete and may be confusing. It should clarify that 'off' prevents browser from automatically filling/completing the field.

    - **autocomplete** (default: on) If 'off' element will not be automatically by the Browser.
    Grammar Issue

    The pattern option description is missing proper punctuation and could be more descriptive about its purpose and usage.

    - **pattern** The pattern (Reg Ex) of value of the form field

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 27, 2024

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    ✅ Documentation should be free of grammatical errors and typos
    Suggestion Impact:The commit fixed the grammatical error in the autocomplete description by adding 'autocompleted' instead of just 'automatically'

    code diff:

    -- **autocomplete** (default: on) If 'off' element will not be automatically by the Browser.
    +- **autocomplete** (default: on) If 'off' element will not be automatically autocompleted by the Browser.

    Fix typo in 'autocomplete' description where "automatically" is incomplete.

    docs/general-concepts/forms-fields/standard-fields/index.md [16]

    -- **autocomplete** (default: on) If 'off' element will not be automatically by the Browser.
    +- **autocomplete** (default: on) If 'off' element will not be automatically completed by the Browser.
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The current description is grammatically incorrect and confusing. Fixing this improves documentation clarity and professionalism, making it easier for developers to understand the autocomplete functionality.

    8
    Documentation should specify the expected type and format for configuration options

    Add missing type information for the 'pattern' option to clarify that it expects a
    regular expression string.

    docs/general-concepts/forms-fields/standard-fields/index.md [28]

    -- **pattern** The pattern (Reg Ex) of value of the form field
    +- **pattern** (type: string) The regular expression pattern that the form field's value must match
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding type information makes the documentation more precise and helps developers understand the expected input format for the pattern option. This reduces potential errors and improves code quality.

    7
    Documentation examples should demonstrate realistic and common use cases

    Add example values for the 'class' option to demonstrate proper usage.

    docs/general-concepts/forms-fields/standard-fields/index.md [29]

    -- **class** Whitespace separated list of classnames which will be added to the input element (e.g. "my-custom-class another-custom-class")
    +- **class** (type: string) Whitespace separated list of classnames which will be added to the input element (e.g. "form-control", "input-lg custom-input")
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: While the existing example is valid, providing more realistic class names helps developers better understand common use cases. However, the current example is already functional, making this a minor improvement.

    5

    @marcorensch marcorensch changed the title Standard Fields index Updated Standard Fields index Dec 27, 2024
    @HLeithner
    Copy link
    Member

    Looks nice, but I miss the data and aria attributes? or are they already documented somewhere?

    @marcorensch
    Copy link
    Contributor Author

    Looks nice, but I miss the data and aria attributes? or are they already documented somewhere?

    Good Point - I've missed that - data attributes are ok - but when testing aria as form field attribute (backend module params for example) it does not get rendered there...

    Added data- Attribute as option including information where it will be added and an example.
    @HLeithner
    Copy link
    Member

    Hmm, ai have to look at the but thought it should be exposed...

    @robbiejackson
    Copy link
    Contributor

    It's important to be clear about the difference between 2 different things:

    • attributes which can be applied to the standard form fields

    • options which can be passed to the renderField() function.

    Most of what you have are attributes, and for these I think that it would be better to include those in the Standard Form Field Attributes page at https://manual.joomla.org/docs/general-concepts/forms-fields/standard-form-field-attributes/ . You can always link to that from the index page, if you think that's appropriate.

    There's a bit of overlap in that the "pattern" and "class" are in both your list and on that Standard Form Field Attributes page.

    Also the "validate" attribute is still used by Joomla - for server-side validation. It's on that page as well.

    However, some of your items are just elements in the $options array of the renderField function. I think that hiddenLabel and hiddenDescription are examples of these. I may be wrong, but looking at the code I don't think that they are general attributes of the form fields.

    I've often thought it poor that these $options weren't properly documented in the API documentation. There are lots of APIs which have a $config or $options parameter with no description of what that parameter can be set to. I think the API documentation is where they should be documented. And then we can refer to that API documentation from the manual.

    @HLeithner
    Copy link
    Member

    thanks @robbiejackson I missed https://manual.joomla.org/docs/next/general-concepts/forms-fields/standard-form-field-attributes/

    I have the feeling that "Form" and "Fields" needs a better structure, not sure in which direction but at least mennu ordering might be needed.

    I would also suggest to structure the standard attributes and similar in away that we automatically include them in each fields for better DX.

    I created a test PR at #357

    @marcorensch
    Copy link
    Contributor Author

    @robbiejackson oooh I've missed that too... but as @HLeithner mentioned if we don't see it we need to change something in the structure.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants