Issue #2473953 by mortendk, rteijeiro, rachel_norfolk, Cottser, LewisNyman: Prefix form-submit classes with js-

Task

Prefix form-submit classes with js- to separate classes needed for JavaScript functionality from those used for styling and make it clear which classes can safely be removed without breaking JavaScript functionality.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing

Steps to test

  1. Navigate to the node/add/article
  2. Add an image to the upload field
  3. Remove the image

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Normal
Unfrozen changes Unfrozen because it mostly just affects templates and JS which are not frozen.
Prioritized changes The main goal of this issue is to improve themer experience and arguably it reduces fragility where JavaScript and markup intersect.
Disruption Shouldn't be too disruptive as it is mostly affecting CSS classes that are added to markup. Themes extending Classy will only have classes added. Themes not extending Classy will have classes removed but they can be added back via template overrides.

User interface changes

None for themes extending Classy. Possible visual changes for other themes.

API changes

n/a

CommentFileSizeAuthor
#11 form_submit-2.png155.97 KBaburrows
#11 form_submit-1.png185.78 KBaburrows
#10 prefix_form_submit-2473953-10.patch3.72 KBstar-szr
#1 interdiff-2473953-form-submit.txt1.31 KBstar-szr
#1 2473953-form-submit-additional.patch3.72 KBstar-szr
#1 2473953-form-submit-split.patch2.98 KBstar-szr

Comments

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2473953-form-submit-split.patch2.98 KB
new 2473953-form-submit-additional.patch3.72 KB
new interdiff-2473953-form-submit.txt1.31 KB

Initial patch split from the parent, some additional changes from grepping, and interdiff between the two.

lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

I manually tested the patch, steps in the issue summary. I also grepped for form-submit and couldn't find anything else to change.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: 2473953-form-submit-additional.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Failed because d.o was down. Thanks Lewis!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Themes extending Classy will only have classes added. Themes not extending Classy will have classes removed but they can be added back via template overrides.

This isn't the case. Stark ends up with both classes.

star-szr’s picture

star-szr
he/him
Primary language English
commented

Sure, if those classes are added in preprocess. In that case it's out of our hands unless we can and want to move the classes to templates.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Issue summary: View changes

Adding suggested commit message.

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Reviewed & tested by the community

Makes sense to me

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Which patch is rtbc?

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new prefix_form_submit-2473953-10.patch3.72 KB

Re-uploading the relevant patch.

aburrows’s picture

aburrows commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new form_submit-1.png185.78 KB
new form_submit-2.png155.97 KB

Tested patch and works as intended

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed d8442c8 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed d8442c8 on 8.0.x 
    Issue #2473953 by mortendk, rteijeiro, rachel_norfolk, Cottser, aburrows...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.