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.

Sign up for GitHub

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

Jump to bottom

FRONT-2476: Form input group created. #73

Merged
merged 7 commits into from
Sep 24, 2021
Merged

FRONT-2476: Form input group created. #73

merged 7 commits into from
Sep 24, 2021

Conversation

Maxfire
Copy link
Contributor

@Maxfire Maxfire commented Sep 21, 2021

@Maxfire Maxfire force-pushed the FRONT-2476 branch 2 times, most recently from 29d510c to c49c111 Compare September 22, 2021 09:48
escuriola
escuriola reviewed Sep 22, 2021
View reviewed changes
Copy link
Contributor

@escuriola escuriola left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some styles missing, for instance form-required has not any special style to mark a field as required, and checkbox left-margin doesn't fit well sometimes.
I'm not sure this must be reviewed now or later in a general form/page theming.

templates/patterns/form_input/form_input.ui_patterns.yml Outdated
label: Text
description: Default input text.
checkbox:
input_type: checkbox
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed

templates/patterns/form_input/form_input.ui_patterns.yml Outdated
label: Checkbox
description: Default checkbox input.
file:
input_type: file
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed

templates/overrides/form/input--textfield.html.twig Outdated
]
%}
{% if attributes.placeholder is empty %}
{% set attributes = attributes.setAttribute('placeholder', 'text here') %}
Copy link
Contributor

Choose a reason for hiding this comment

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

This default text should be translatable

escuriola
escuriola previously approved these changes Sep 22, 2021
View reviewed changes
drishu
drishu requested changes Sep 23, 2021
View reviewed changes
Copy link
Contributor

@drishu drishu left a comment

Choose a reason for hiding this comment

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

  • the helper text has the correct class, and even though not a div, visually its ok
  • valid-feedback and invalid-feedback are handled differently in drupal so we won't touch those
  • if validation error is present the message is presented on top in the messages area and the border is collored red as it should be (in netlify its not and that is a mistake I feel)

templates/overrides/form/form-element--checkbox.html.twig Outdated Show resolved Hide resolved
templates/overrides/form/input--textfield.html.twig Outdated Show resolved Hide resolved
templates/overrides/form/input--textfield.html.twig Outdated Show resolved Hide resolved
drishu
drishu previously approved these changes Sep 23, 2021
View reviewed changes
@drishu drishu merged commit 77ec626 into 1.x Sep 24, 2021
@drishu drishu deleted the FRONT-2476 branch September 24, 2021 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brummbar brummbar brummbar approved these changes

@drishu drishu drishu approved these changes

@escuriola escuriola escuriola left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Maxfire @escuriola @brummbar @drishu