Problem/Motivation

There is an unused duplicate variable 'dropdowns' in duplicateGroupsOperator() in views-admin.js.

Proposed resolution

Remove. Patch below.

Remaining tasks

None.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#25 interdiff-duplicate_unused-2302515-19-25.txt818 byteshog
#25 duplicate_unused-2302515-25.patch539 byteshog
#21 interdiff-duplicate_unused-2302515-21.txt1.54 KBhog
#21 duplicate_unused-2302515-21.patch1.33 KBhog
#19 duplicate_unused-2302515-19.patch1.21 KBeleleka
#15 duplicate_unused-2302515-15.patch675 byteskostyashupenko
#10 duplicate_unused-2302515-10.patch1.21 KBcamoa
#3 drupal-views-duplicate-dropdowns-var-2302515-3.patch1.24 KBelijah lynn
#1 drupal-views-duplicate-dropdowns-var-2302515-1.patch1.24 KBelijah lynn

Comments

elijah lynn’s picture

elijah lynn
Location Portland, Oregon
commented
Status: Active » Needs review
StatusFileSize
new drupal-views-duplicate-dropdowns-var-2302515-1.patch1.24 KB

Status: Needs review » Needs work

The last submitted patch, 1: drupal-views-duplicate-dropdowns-var-2302515-1.patch, failed testing.

elijah lynn’s picture

elijah lynn
Location Portland, Oregon
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-views-duplicate-dropdowns-var-2302515-3.patch1.24 KB

Ahh, this patch #2301953: Update 'element-invisible' and 'element-hidden' to valid Drupal 8 CSS classes just got applied causing it to fail. Re-rolled.

elijah lynn’s picture

elijah lynn
Location Portland, Oregon
commented
Related issues: +#2302515: Duplicate/unused variable 'dropdowns' in duplicateGroupsOperator() in views-admin.js
linl’s picture

linl commented
Issue tags: +JavaScript
Related issues: -#2302515: Duplicate/unused variable 'dropdowns' in duplicateGroupsOperator() in views-admin.js

Adding js tag.

droplet’s picture

droplet commented
Status: Needs review » Needs work

I think it's more than "dropdowns"

http://cgit.drupalcode.org/drupal/tree/core/modules/views_ui/js/views-ad...

"titleRow" and some other expressions.

elijah lynn’s picture

elijah lynn
Location Portland, Oregon
commented

Yeah, there are a bunch more in that file but I wasn't sure if I should make separate issues for each one?

droplet’s picture

droplet commented

that's fine to clean up in same issue, we did a lot of these cleanups before: #1415788: Javascript winter clean-up

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue tags: +Novice
camoa’s picture

camoa commented
StatusFileSize
new duplicate_unused-2302515-10.patch1.21 KB

This should move the cleanup further.

I think that will do it as a clean up, there is some repeated code but it is not repeated functionality.

camoa’s picture

camoa commented
Status: Needs work » Needs review

droplet queued 10: duplicate_unused-2302515-10.patch for re-testing.

droplet’s picture

droplet commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
Alka Kumari’s picture

Alka Kumari commented
Assigned: Unassigned » Alka Kumari
Issue tags: +drupalconasia2016
kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new duplicate_unused-2302515-15.patch675 bytes

Re-roll of patch #10

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

still needs proper re-roll

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented

Not sure that we still need re-roll, because there was an auto merge. Better to make a patch

gvso’s picture

gvso
Primary language Spanish
commented

If we remove dropdowns = this.operator, drow dropdowns = dropdowns.add(fakeOperator); will produce an error.

eleleka’s picture

eleleka commented
Assigned: Alka Kumari » Unassigned
Status: Needs work » Needs review
StatusFileSize
new duplicate_unused-2302515-19.patch1.21 KB

Rerolled patch #10
There is some confusion in line numbers

droplet’s picture

droplet commented
Status: Needs review » Needs work

Please also check the code and do manual tests during reroll.

hog’s picture

hog commented
Status: Needs work » Needs review
StatusFileSize
new duplicate_unused-2302515-21.patch1.33 KB
new interdiff-duplicate_unused-2302515-21.txt1.54 KB

Edited last path, as it not worked correctly. Added styles for filter sort table header for pretty display. Patch work ok now as i see, but i find new error after add new group:
Uncaught TypeError: Cannot read property 'width' of undefined. In function Drupal.dialog~resetSize

droplet’s picture

droplet commented
Status: Needs review » Needs work

We shouldn't do multiple changes in one issue. Please open an issue for CSS part.

i find new error after add new group:
Uncaught TypeError: Cannot read property 'width' of undefined. In function Drupal.dialog~resetSize

is it caused by this patch ?

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Issue tags: -Needs reroll
hog’s picture

hog commented

@droplet, js bug is in 8.0.x branch also.
About multiple changes i separate it now.

hog’s picture

hog commented
Status: Needs work » Needs review
StatusFileSize
new duplicate_unused-2302515-25.patch539 bytes
new interdiff-duplicate_unused-2302515-19-25.txt818 bytes
zeeshan_khan’s picture

zeeshan_khan commented
Status: Needs review » Reviewed & tested by the community

Looks good to me ;)
Thanks @HOG

hog’s picture

hog commented
Related issues: +#2675522: Styling for Views UI rearrange filters

Filed follow-up issue for CSS #2675522: Styling for Views UI rearrange filters

hog’s picture

hog commented

How to test:

  1. Open link: admin/structure/views/view/content
  2. On filter group click on arow (after Add lunk)
  3. Select 'Add or reange'
  4. You see popup: 'Page: Rearrange filter criteria'

  • catch committed ff06c79 on 8.1.x 
    Issue #2302515 by HOG, Elijah Lynn, kostyashupenko, camoa, eleleka:...
catch’s picture

catch
he/him
Primary language English
commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x, thanks!

This is just clean-up so not eligible for 8.0.x.

Status: Fixed » Closed (fixed)

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