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.

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

Ensure that only one filter dropdown gets rendered per action type #11893

Merged
merged 2 commits into from
Oct 17, 2016

Conversation

AparnaKarve
Copy link
Contributor

When the Action dropdown values are changed one after the other, say, from VM Analysis to Template Analysis to Host Analysis , the Filter dropdown from the earlier Action dropdown value still lingers around.

See the below screenshot :
screen shot 2016-10-12 at 4 06 41 pm

This PR addresses the above bug.

IIRC, this used to work before so I'm going to attribute this behavior to the latest version of angular?(maybe)

@AparnaKarve
Copy link
Contributor Author

@himdel I encountered the above bug while testing your PR #11865.
If this looks good, how about we merge this first so that I can test your PR properly?

@AparnaKarve AparnaKarve reopened this Oct 13, 2016
@himdel
Copy link
Contributor

himdel commented Oct 13, 2016

Oh wow, that's a fun bug, thanks :).

The cause is the new patternfly version (3.8 or 3.9 IIRC) - they changed the selectpicker element - previously selectpicker init would turn a select into essentially <select class=hidden></select><div>magic</div>, whereas now it becomes <div>magic<select></select></div>. Which means that ng-switch does hide the select element, but not the div that contains it, which is the only thing that's visible. OTOH ng-if works because it seems it holds a reference to the right element and so manages to destroy the div too.

Which means the Right Fix is probably more along the lines of updating selectpicker-for-select-tag to destroy that div when it ceases to exist (something like...

scope.$on('$destroy', function () {
  element.selectpicker('destroy');
});

... in the link function) or replacing selectpicker-for-select-tag with pf-select, which should already be handling that.

(Or, if that sounds wrong for some reason, another alternative would be to put the ng-switch-when on an element right above the select, instead of directly on the select.)

((Or, we could merge this one for now, but I don't like having both ng-if and ng-switch at the same time..))

@AparnaKarve
Copy link
Contributor Author

So technically this is a selectpicker bug. I would rather have them fix this.
But for now, as far as possible I do not want to touch selectpicker-for-select-tag since it's used in other places.

The ng-switch-when approach might be OK but that would mean more # of lines changed in the haml (there are at least 10 occurrences that we are talking about)

Hence I vote for the ng-if approach :)
We can always revisit this, once the selectpicker bug is fixed.

@himdel
Copy link
Contributor

himdel commented Oct 13, 2016

So technically this is a selectpicker bug. I would rather have them fix this.

Oh not quite. It's in how we're using selectpicker from angular, the JS only implementation does not have the problem in pure-JS context. (And the patternfly angular directive does handle it fine.)

That said, I'm OK with the ng-if approach, if you remove the switch :)

@AparnaKarve
Copy link
Contributor Author

AparnaKarve commented Oct 13, 2016

Ok, a couple things you should know:

  • pf-select does not work
  • ng-if will not work without ng-switch-when

If something worked before, and is not working anymore, then I consider it to be a bug...unless they replaced the logic with a better approach -- which does not seem to be the case here.
More importantly, pf-select which is patternfly's own directive does not seem to be working in this situation either. So yes, this is a PF bug.

@himdel
Copy link
Contributor

himdel commented Oct 14, 2016

Ha, yup, you're completely right, it's a bug in pf-select. But fixing it in pf-select won't help us if we're using selectpicker-for-select-tag, so we'd still need to fix it there.

You're also right that ng-if doesn't work without that ng-switch-when, sorry, I don't know what I tested yesterday then, but apparently not this O:-). But that's bad, since it means this works only by accidental magic, which we definitely don't want. (Maybe the switch creates an extra context that helps the if to remove the right element or something, like virtually wrapping it in an extra element, idk.)

So we need to fix it in patternfly, and then .. well, your decision, I think doing the same fix for selectpicker-for-select-tag as for that pf-select should be harmless and in fact fix all the other selects too, if there are any more with this bug, but we can either do that, or move to the fixed pf-select here and handle the rest of the selects only where needed.

(Discussion of the original selectpicker change is in #10509)

angular-patternfly PR: patternfly/angular-patternfly#329

@AparnaKarve AparnaKarve force-pushed the fix_schedule_filter_type_dropdown branch from 5581a31 to cfa757e Compare October 14, 2016 18:57
@AparnaKarve
Copy link
Contributor Author

@himdel I've made changes to destroy the selectpicker div in the directive.

Addressed a similar issue in cfa757e -- removed ng-if from there.

Please review. Thanks.

@himdel
Copy link
Contributor

himdel commented Oct 17, 2016

@AparnaKarve thanks! Works like a charm, merging :)

Introduced in #10509 - adding euwe/yes

@himdel himdel merged commit 3947eb2 into ManageIQ:master Oct 17, 2016
@himdel himdel added this to the Sprint 48 Ending Oct 24, 2016 milestone Oct 17, 2016
@AparnaKarve AparnaKarve deleted the fix_schedule_filter_type_dropdown branch October 18, 2016 00:02
chessbyte pushed a commit that referenced this pull request Oct 18, 2016
…ropdown

Ensure that only one filter dropdown gets rendered per action type
(cherry picked from commit 3947eb2)
@chessbyte
Copy link
Member

Euwe Backport details:

$ git log -1
commit cccccf420742ce69b33773f66d05ed3f33ef715a
Author: Martin Hradil <himdel@seznam.cz>
Date:   Mon Oct 17 12:36:14 2016 +0000

    Merge pull request #11893 from AparnaKarve/fix_schedule_filter_type_dropdown

    Ensure that only one filter dropdown gets rendered per action type
    (cherry picked from commit 3947eb22d98b7eed4135b50d3f95b6d81b67c6a6)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants