PTV-1620/PTV-1845 fix kbaseTabs #3117
Conversation
|
Still in progress, but it looks like draft PRs don't invoke GHA, and I need to iterate on tests and code quality too! |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #3117 +/- ##
========================================
Coverage 73.37% 73.37%
========================================
Files 36 36
Lines 3917 3917
========================================
Hits 2874 2874
Misses 1043 1043 Continue to review full report at Codecov.
|
…examples to gallery [PTV-1620][PTV-1845]
…out the code [PTV-1620][PTV-1845]
|
SonarCloud Quality Gate failed.
|
| 'use strict'; | ||
| return KBWidget({ | ||
| name: 'kbaseTabs', | ||
|
|
||
| version: '1.0.0', | ||
|
|
||
| _accessors: ['tabsHeight'], |
There was a problem hiding this comment.
tabsHeight not used anywhere, so removed.
| canDelete: false, | ||
| borderColor: 'lightgray', |
There was a problem hiding this comment.
borderColor not used anywhere, so removed.
|
|
||
| options: { | ||
| tabPosition: 'top', |
There was a problem hiding this comment.
tabPosition not used anywhere, endnote actually implemented in the widget, and not supported by bootstrap 3 afaik, so removed.
| @@ -64,167 +64,208 @@ define(['kbwidget', 'bootstrap', 'jquery', 'kbaseDeletePrompt'], ( | |||
| }, | |||
|
|
|||
| appendUI: function ($elem, tabs) { | |||
| if (tabs == undefined) { | |||
| if (typeof tabs === 'undefined') { | |||
There was a problem hiding this comment.
safer test for undefinedness.
| } | ||
| }, | ||
|
|
||
| addTab: function (tab) { | ||
| if (tab.canDelete == undefined) { | ||
| if (typeof tab.canDelete === 'undefined') { |
There was a problem hiding this comment.
safer undefinedness test
| const $tab = $('<div role="tabpanel"></div>').addClass('tab-pane fade'); | ||
|
|
||
| $tab.hasContent = false; | ||
| const $tabPanel = $('<div role="tabpanel"></div>').addClass('tab-pane fade'); |
There was a problem hiding this comment.
chose a better name; with all the "tab"s floating around (including "tab.tab"!), it is important to be more specific in naming things.
| } | ||
|
|
||
| if (this.options.border) { |
There was a problem hiding this comment.
removing border feature, as unused.
| const $widget = this; | ||
|
|
||
| // Create the tab button itself. | ||
| const $navButton = $('<a>') |
There was a problem hiding this comment.
in order to make debugging and understanding the code construction better, I refactored out some large swathes of independent code like this.
| }, | ||
|
|
||
| removeTab: function (tabName) { | ||
| const $tab = this.data('tabs')[tabName]; | ||
| const $nav = this.data('nav')[tabName]; | ||
| const deleteNav = () => { |
There was a problem hiding this comment.
extracted the deletion tab nav and panel as they are re-used, and also used separately.
| $nav.remove(); | ||
|
|
||
| this.data('tabs')[tabName] = undefined; | ||
| this.data('nav')[tabName] = undefined; | ||
| }, | ||
|
|
||
| deletePrompt: function (tabName) { |
There was a problem hiding this comment.
simplified as there was no need to have an anonymous callback function call a method.
| }, | ||
|
|
||
| deletePrompt: function (tabName) { | ||
| const $deleteModal = new kbaseDeletePrompt($('<div></div>'), { |
There was a problem hiding this comment.
simplified node creation like this in several places.
ialarmedalien
changed the base branch from
develop
to
PTV-1620-browser-freeze
|
|
||
| The primary customizations are: | ||
| - tabs can be set as deletable | ||
| - |
There was a problem hiding this comment.
Is that the only customisation?
| var $tabs = new kbaseTabs($('#tabs'), { | ||
| tabPosition : 'bottom', //or left or right or top. Defaults to 'top' | ||
| canDelete : true, //whether or not the tab can be removed. Defaults to false. | ||
| tabPosition : 'bottom', // or 'left' or 'right' or 'top'. Defaults to 'top' |
There was a problem hiding this comment.
looks like this was removed below
|
|
||
| renderDeleteTabButton: function (tab) { | ||
| return $('<button>') | ||
| .addClass('btn btn-default btn-xs') |
There was a problem hiding this comment.
can you give this button a unique class (btn-delete?) and add all the css there, rather than having it hardcoded here?
There was a problem hiding this comment.
Not really hard coded is it? These are "standard" bootstrap button classes, meant to be combined like this.
Maybe I'm not understanding your point.
| expect($host.text()).toContain('Foo'); | ||
| expect($host.text()).toContain('BAR'); | ||
| expect($host.text()).toContain('Baz'); | ||
| expect($host.text()).not.toContain('Fuzz'); |
There was a problem hiding this comment.
Can you make these tests more specific so they differentiate between what's in the tab label vs what's in the tab content?
There was a problem hiding this comment.
Yes! There is actually a change after this which narrows the scope to the nav tabs and pane separately, not in the PR yet.
| const $host = $(document.createElement('div')); | ||
| new KBaseTabs($host, { | ||
| tabs: [ | ||
| { | ||
| tab: 'Foo', | ||
| content: 'BAR', | ||
| }, | ||
| { | ||
| tab: 'Baz', | ||
| content: 'Fuzz', | ||
| }, | ||
| ], | ||
| }); | ||
| expect($host.text()).toContain('Foo'); | ||
| expect($host.text()).toContain('BAR'); | ||
| expect($host.text()).toContain('Baz'); | ||
| expect($host.text()).not.toContain('Fuzz'); |
There was a problem hiding this comment.
since this is repeated several times, can you extract it out into a function?
There was a problem hiding this comment.
If you look closely you'll see that each test case configures the tabs differently.
| @@ -0,0 +1,44 @@ | |||
| # Demos | |||
|
|
|||
| These demos are static html pages which demonstrate some relatively simple and isolatable aspect of the Narrative. This simple demo system is particularly suitable for ui components. | |||
There was a problem hiding this comment.
This looks a whole lot like Storybook. So why not just use Storybook?
There was a problem hiding this comment.
Just wanted something simple to be able to preview components w/out having to fire up the narrative and without having to introduce something like storybook to the codebase. I'll just remove this. It is useful for me, and I thought it might be for others as well.
There was a problem hiding this comment.
I'm definitely not against something like this for the Narrative, this does seem a bit out of scope for this PR, though, and could use some discussion.
Thanks for removing for now.
There was a problem hiding this comment.
No problem. It was the only way I could actually use the tab control with all combinations of options, since they don't actually all exist in the codebase; so it served its purpose for me at least :)
|
Back onto this PR - which BTW was in "fake draft" status, i.e. not ready for review. (The reason for not being in official draft status is that the GHA doesn't run for drafts, and I need to be able to iterate on the code quality, etc. before releasing it for review, as well as fold in changes from PR 3116...) |
Description of PR purpose/changes
PR IN PROGRESS
The venerable "kbaseTabs" widget is behaving incorrectly in the kbaseExpressionConditionsetBaseWIdget.
Specifically, it is not showing the previous tab when a dynamic tab is closed, as is done when clicking on a cluster.
While fixing the kbaseTabs widget, and expanding the tests to cover additional use case, I discovered at least one additional latent bug (or at least unexpected behavior), a failure to activate the first tab if no tabs are set explicitly as active.
In addition, there were several features in this already rather complex widget that are not used anywhere in the codebase, and have been removed. They may have been used in the past, but I suspect they were "nice to have" features that were simply added for future utility. However, retaining them would require adding tests to cover them, yet yield no benefit as they are unused.
Finally, tests were expanded to cover common use cases, including the primary issue fixed by this PR.
Jira Ticket / Issue
https://kbase-jira.atlassian.net/browse/PTV-1620
Added the Jira Ticket to the title of the PR (e.g.
DATAUP-69 Adds a PR template)Testing Instructions
New tests were added to test/unit/spec/widgets/kbaseTabs-spec.js to cover the use case of closing a dynamically added tab.
To evaluate this manually, spin up a narrative from this PR, create a narrative, add a FeatureClusters object, and open the viewer.
There is also a gallery demonstrating usage of kbaseTabs. This is a new feature in the narrative, but "out of the way" - it does not change the actual narrative runtime. It is a set of static html pages with support for building gallery demo pages. Each gallery demo page imports code it wants to demonstrate. In a running narrative it is available at url like:
https://ci.kbase.us/narrative/static/kbase/gallery/index.html
Dev Checklist:
Updating Version and Release Notes (if applicable)