Skip to content

Disable automatically generating hints, placeholders and labels for simple_form#7539

Merged
mitchellhenke merged 2 commits intomainfrom
mitchellhenke/disable-simple-form-placeholder-hint-label
Jan 4, 2023
Merged

Disable automatically generating hints, placeholders and labels for simple_form#7539
mitchellhenke merged 2 commits intomainfrom
mitchellhenke/disable-simple-form-placeholder-hint-label

Conversation

@mitchellhenke
Copy link
Contributor

🛠 Summary of changes

A follow up to #7537 that is intended to address the root problem of implicitly generating labels. This is in part relying on existing tests to find issues like those in #7537. I'm not sure if we want to add some defensiveness by requiring the label: parameter on things like ValidatedFieldComponent.

The secondary benefit to this is a minor performance improvement when using simple_form_for. simple_form attempts to find translations for each hint, placeholder and label it may create which results in some additional work. Preliminary testing shows a 2-3% reduction in memory allocation for GET /. I'm not expecting a noticeable effect in response time.

@mitchellhenke mitchellhenke requested a review from aduth December 23, 2022 21:08
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/disable-simple-form-placeholder-hint-label branch 3 times, most recently from e726dbc to 660d6c2 Compare December 23, 2022 21:33
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/disable-simple-form-placeholder-hint-label branch from 660d6c2 to d5495f2 Compare January 3, 2023 22:36
@mitchellhenke mitchellhenke marked this pull request as ready for review January 3, 2023 22:51
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I noticed that the label_text in the legend wrapper appears to add its own label as well:

b.wrapper :legend, tag: 'legend', class: legend_class do |ba|
ba.use :label_text
end

For example, commenting this line would cause a default label to be shown:

label: t('forms.registration.labels.email_language'),

Should that be optional as well?

Also, I see it's documented in the SimpleForm documentation, but I'm not sure what default hints and placeholders were actually doing, if anything?

…imple_form

changelog: Internal, Forms, Disable automatically generating hints and placeholders and labels for simple_form
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/disable-simple-form-placeholder-hint-label branch from d5495f2 to 41e21b4 Compare January 4, 2023 16:07
@mitchellhenke
Copy link
Contributor Author

LGTM 👍

I noticed that the label_text in the legend wrapper appears to add its own label as well:

b.wrapper :legend, tag: 'legend', class: legend_class do |ba|
ba.use :label_text
end

For example, commenting this line would cause a default label to be shown:

label: t('forms.registration.labels.email_language'),

Should that be optional as well?

Great catch, yes, updated.

Also, I see it's documented in the SimpleForm documentation, but I'm not sure what default hints and placeholders were actually doing, if anything?

I'm not sure, it's hard to test when it would come into effect, so I may have over-applied it.

@mitchellhenke mitchellhenke merged commit e7b8fb5 into main Jan 4, 2023
@mitchellhenke mitchellhenke deleted the mitchellhenke/disable-simple-form-placeholder-hint-label branch January 4, 2023 20:58
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.

3 participants