Implemented select/deselect option for tags dropup in graph editor #138
Conversation
There was a problem hiding this comment.
Cool PR @Mr-Nobody99 - i like this a lot!
Couple of quick things:
- When all tags are unselected, the select state shows
select_all. I dropped a comment inline in the code about how I think we could resolve this, but curious to hear what you're thinking too.
- It probably makes sense to add this same component to the
resourcesandpackagesselectors too! You think there's a straightforward way to this option available on each of the dropdowns in the DAG view?
Nice work so far!
| @@ -173,6 +173,12 @@ <h6> | |||
| <ul | |||
| class="dropdown-menu" | |||
| ng-show="isVisible('tags')"> | |||
| <li ng-click="onItemSelect('tags', 'select_all', $event)"> | |||
| <span>Select All</span> | |||
There was a problem hiding this comment.
Maybe we can make this bold/black text instead of light blue? How about something like
| <span>Select All</span> | |
| <strong class="text-dark">Select All</strong> |
There was a problem hiding this comment.
sure that's no problem, I left it as default link color with the thought that that might distinguish it a bit from the rest of the list. But I think just doing bold is good as well.
| @@ -173,6 +173,12 @@ <h6> | |||
| <ul | |||
| class="dropdown-menu" | |||
| ng-show="isVisible('tags')"> | |||
| <li ng-click="onItemSelect('tags', 'select_all', $event)"> | |||
There was a problem hiding this comment.
by treating this as a regular item that can be selected, we can find ourselves in some funky states! Check this example out, for instance:
Instead, I think we might just want to add a new method in the scope called something like toggleSelectAll('tags'). This method would either unselect all tags, or make all tags selected depending on the state. I think that would help clean up some of the logic added in graph-launcher.js too!
Let me know if you have any thoughts or questions about an approach like this one :)
There was a problem hiding this comment.
I definitely see your point, I can change it to a separate function. That is what I had considered at first, I thought the current was just less intrusive.
…lect all option to resource_type and package dropup as well.
| @@ -115,6 +115,12 @@ <h6> | |||
| <ul | |||
| class="dropdown-menu" | |||
| ng-show="isVisible('resource_types')"> | |||
| <li ng-click="onSelectAll('resource_types', allSelected ? false : true, $event)"> | |||
There was a problem hiding this comment.
Just curious - why do allSelected ? false : true instead of !allSelected?
There was a problem hiding this comment.
No particular reason I didn't think of that, but that is a bit cleaner I'll change it.
|
This is really excellent! Thanks for making those changes - I think this works really well now. Can you just:
After that, this should be ready to merge! |
|
@drewbanin Thanks a lot happy to help, I just finished those changes. |
CHANGELOG.md
Outdated
| @@ -1,4 +1,8 @@ | |||
| ## dbt 0.19.0 (Release TBD) | |||
| - Add select/deselect option in DAG view dropups. ([docs#98](https://github.com/fishtown-analytics/dbt-docs/issues/98)) | |||
|
|
|||
| Contributors: | |||
There was a problem hiding this comment.
can you just add yourself as a contributor in the existing section below? Looks like we have two contributor sections for the 0.19.0 release now :)
There was a problem hiding this comment.
Ahh, yep just corrected it thanks.
|
Woot! thanks a lot |
resolves #98
Description
After seeing the open issue for a select/deselect all option for tags in the graph editor, I thought that makes a lot of sense.
So I implemented it, the changes are in the graph-launcher. I added a new list item in the html and some conditions in onItemSelect() to handle the change.
Checklist
CHANGELOG.mdand added information about my change to the "dbt next" section.