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

Dataup 224 add check boxes to staging #1874

Merged
merged 50 commits into from
Nov 2, 2020

Conversation

leia-sefkin
Copy link
Contributor

@leia-sefkin leia-sefkin commented Oct 17, 2020

Description of PR purpose/changes

Screen Shot 2020-10-16 at 6 19 10 PM

Finally ready in all of its glory - multi-select checkboxes. Phew! It's been a wild ride - thanks for coming along y'all

Requirements:

  • Make file type selection box red for each uploaded file when a file type has NOT been selected. (Without an autodetect implemented, all files will show a red file type selector by default). The checkbox for that row is also default disabled.

  • Until any file type is selected the check all checkbox is disabled

  • When a file type is selected for that row, the red box color goes back to the original (grey) color and checkboxes are enabled.

  • Import Selected button is disabled until a file is selected.

  • On selection of file(s) and clicking import selected new importers will be kicked off. One for each file.

Jira Ticket / Issue

https://kbase-jira.atlassian.net/browse/DATAUP-224

Testing Instructions

  • Tests pass locally, to isolate run stagingAreaViewer-spec.js
  • Changes available by spinning up a local narrative:
  • Navigate to the data import tab
  • Make sure you have at least one file added
  • Before selecting data type the dropdown should be red placeholder text "Select a type"
  • On clicking in the border and text color should change to black
  • If you click away without selecting a type text color should return to placeholder state
  • If you select a type the text and border should stay black
  • Checkboxes should be enabled
  • On checking a checkbox the import selected button should become enabled

Dev Checklist:

  • My code follows the guidelines at https://sites.google.com/truss.works/kbasetruss/development
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@coveralls
Copy link

coveralls commented Oct 17, 2020

Coverage Status

Coverage decreased (-0.4%) to 16.582% when pulling 813e8b9 on DATAUP-224-add-check-boxes-to-staging into a30f5aa on truss.

@bio-boris bio-boris changed the base branch from develop to truss October 17, 2020 04:59
Copy link
Contributor

@eamahanna eamahanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the comments AJ left. Once those changes are made and I can run the branch locally (when things are up and running), I am happy to approve :)

Copy link
Member

@briehl briehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple pretty picky points, but this looks great! Nice work wrangling Select2!

@codecov
Copy link

codecov bot commented Oct 30, 2020

Codecov Report

Merging #1874 into truss will increase coverage by 1.04%.
The diff coverage is 45.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##            truss    #1874      +/-   ##
==========================================
+ Coverage   12.69%   13.74%   +1.04%     
==========================================
  Files         405      414       +9     
  Lines       43739    44756    +1017     
==========================================
+ Hits         5551     6150     +599     
- Misses      38188    38606     +418     
Impacted Files Coverage Δ
...idgets/narrative_core/kbaseNarrativeManagePanel.js 0.53% <ø> (+<0.01%) ⬆️
kbase-extension/static/kbase/js/kbaseNarrative.js 39.57% <37.38%> (ø)
...widgets/narrative_core/upload/stagingAreaViewer.js 63.05% <54.94%> (+0.88%) ⬆️
...js/widgets/appWidgets2/input/select2ObjectInput.js 80.12% <0.00%> (-0.63%) ⬇️
...xtension/static/kbase/js/kbaseNarrativePrestart.js 45.63% <0.00%> (ø)
kbase-extension/static/kbase/js/narrativeConfig.js 67.69% <0.00%> (ø)
...ension/static/kbase/js/widgetMaxWidthCorrection.js 16.66% <0.00%> (ø)
kbase-extension/static/kbase/js/tour.js 46.00% <0.00%> (ø)
kbase-extension/static/kbase/js/kbwidget.js 46.66% <0.00%> (ø)
kbase-extension/static/kbase/js/narrativeLogin.js 61.45% <0.00%> (ø)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7539b8d...69b2bba. Read the comment docs.


const headerCheckbox = $targetNode.find('#staging_table_select_all');

//TODO: for some weird reason the header checkbox isn't showing as enabled, even though the click event fires. not sure what is going on here
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is one part of the tests i couldn't quite figure out.

when i set the dropdown value above on line 293, this triggers the select2:select event in stagingAreaViewer on 491. i am able to confirm this event gets fired, as the above check to make sure the checkbox input is enabled works.

however for some reason the updates to the header "select all" checkbox don't get applied, even though they are in the same method call, on line 506.

the test still "works" because the element exists, but it's not really a fair test of the functionality that we are expecting (that the checkbox should be enabled). any advice is welcome

@leia-sefkin
Copy link
Contributor Author

leia-sefkin commented Oct 30, 2020

Note that Codacy is complaining about having an !important property on one of my css declarations, but that's needed to override a dataTables inline css style

Copy link
Contributor

@eamahanna eamahanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice job.

Copy link
Member

@briehl briehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Nice work!

@sonarcloud
Copy link

sonarcloud bot commented Nov 2, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 1 Security Hotspot to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@leia-sefkin leia-sefkin merged commit adf68e3 into truss Nov 2, 2020
@leia-sefkin leia-sefkin deleted the DATAUP-224-add-check-boxes-to-staging branch November 2, 2020 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants