Updated: Comment #N

Problem/Motivation

we dont use the id in Drupal unless its absolutely nessesary - its a pita for frontenders to work with & is unessasary
markupelements that needs a javascript behaviour must be prefixed with js- class to make it clear for the themer as stated here
https://drupal.org/node/1887918#separate-concerns or even better become a data-xxx so the css masters by accident removes the js-foo "cause its not doing anything"

Proposed resolution

remove the id="progress" from the template use a data-foo instead

before:

<div id="progress" class="progress">
  {% if label %}
    <div class="progress__label">{{ label }}</div>
  {% endif %}
  <div class="progress__track"><div class="progress__bar" style="width: {{ percent }}%"></div></div>
  <div class="progress__percentage">{{ percent }}%</div>
  <div class="progress__description">{{ message }}</div>
</div>

after:

<div class="progress"  data-drupal-progress>
  {% if label %}
    <div class="progress__label">{{ label }}</div>
  {% endif %}
  <div class="progress__track"><div class="progress__bar" style="width: {{ percent }}%"></div></div>
  <div class="progress__percentage">{{ percent }}%</div>
  <div class="progress__description">{{ message }}</div>
</div>

Remaining tasks

test it

User interface changes

none

API changes

none

none

CommentFileSizeAuthor
#19 core-markup-progress-2091971-19.patch934 bytesnod_
#17 remove-id-progress-bar-2091971-17.patch920 bytesrainbowarray
#13 drupal-2091971.patch923 byteskatzilla

Comments

tlattimore’s picture

tlattimore commented

Oh my. id="progress" class="progress" - that is just dumb in my opinion. The proposed solution is much cleaner.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: use js- prefix class instead of ID in progress-bar.html.twig » Remove ID in progress-bar.html.twig
Issue tags: +JavaScript

and use a data- attribute for the JS, also, tag.

dbazuin’s picture

dbazuin commented

I agree sounds perfect. Let's do it.

mortendk’s picture

mortendk commented

So were talking about ripping out class="js-progress" and instead add it in as a data-xx
that sounds even better so its completely seperated

nod_’s picture

nod_
Primary language French
Location Lille
commented

<div class="progress" data-drupal-progress> then assign to me :)

dbazuin’s picture

dbazuin commented

Should that not be

 <div class="progress" data-drupal="progress">
dbazuin’s picture

dbazuin commented
Issue summary: View changes

Updated issue summary.

mortendk’s picture

mortendk commented

updated the info but cant assign it to nod_ ?
can i haz epic power ?

nod_’s picture

nod_
Primary language French
Location Lille
commented

Could be, but then data-drupal will end up with completely unpredictable values and it'll be a mess when it's used for the message area and this or that.

We just need the data attribute to "tag" a "tag", we don't need a value. It also means that the js selector becomes '[data-drupal="progress"]' instead of the much nicer '[data-drupal-progress]'.

So, no :)

nod_’s picture

nod_
Primary language French
Location Lille
commented

@mortendk, once the twig patch is done and I just need to fix the JS :)

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue tags: -JavaScript

Actually, that's even simpler. This template is only used on non-js pages. We got the equivalent more or less hardcoded in the JS.

So this patch has no JS impact and doesn't need any data attribute. You can just remove the progress id and all will be well.

dbazuin’s picture

dbazuin commented

So you would have used a data attribute without giving it a value?
Hmm hell why not :-)

dbazuin’s picture

dbazuin commented
Issue summary: View changes

Updated issue summary.

katzilla’s picture

katzilla
she/her
Primary language German
Location Göttingen, Germany
commented
Assigned: Unassigned » katzilla
Issue summary: View changes
Issue tags: +#SprintWeekend2014

I will have a look at this...

katzilla’s picture

katzilla
she/her
Primary language German
Location Göttingen, Germany
commented
Status: Active » Needs review
StatusFileSize
new drupal-2091971.patch923 bytes

Simply removing the ID did not do the trick, because batch.js was also picking the ID from progress. Changed this to data-attribute now. Please review.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Can you remove the div from the selector? Down the line we might end up using the progress element and it'd be a shame to break the js just beacuse of that. Thanks :)

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue summary: View changes
Status: Needs review » Needs work
rainbowarray’s picture

rainbowarray
she/her
Primary language English
Location Twin Cities, Minnesota
commented

Working on a patch to take the div out of the selector.

rainbowarray’s picture

rainbowarray
she/her
Primary language English
Location Twin Cities, Minnesota
commented
Status: Needs work » Needs review
StatusFileSize
new remove-id-progress-bar-2091971-17.patch920 bytes

Here's a patch update with the div removed from the selector.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +JavaScript

Perfect, thanks!

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue summary: View changes
StatusFileSize
new core-markup-progress-2091971-19.patch934 bytes

This is a reroll because the indentation standard changed for JavaScript files. No need for commit credit because of this reroll.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: core-markup-progress-2091971-19.patch, failed testing.

rainbowarray’s picture

rainbowarray
she/her
Primary language English
Location Twin Cities, Minnesota
commented
Status: Needs work » Needs review

I ran the render test that failed on my local dev with the patch applied, and the test passed. So, setting testbot to rerun this.

rainbowarray’s picture

rainbowarray
she/her
Primary language English
Location Twin Cities, Minnesota
commented

19: core-markup-progress-2091971-19.patch queued for re-testing.

rainbowarray’s picture

rainbowarray
she/her
Primary language English
Location Twin Cities, Minnesota
commented

Tests passed, good to go?

droplet’s picture

droplet commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

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

Committed e044708 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

alimac’s picture

alimac commented
Issue tags: -#SprintWeekend2014 +SprintWeekend2014

Minor tag cleanup - please ignore.