-
Notifications
You must be signed in to change notification settings - Fork 4
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
DUOS-2014[risk=no] Updated Researcher Actions functionality to account for DarCollectionSummary #1744
Merged
DUOS-2014[risk=no] Updated Researcher Actions functionality to account for DarCollectionSummary #1744
Changes from 52 commits
Commits
Show all changes
55 commits
Select commit
Hold shift + click to select a range
78cad95
Merge branch 'develop' into DUOS-1905
connorlbark 92a91e3
update researcher console dar table + actions
connorlbark 30bd225
naming nit
connorlbark 2b00ec3
fix tests
connorlbark feb667d
Apply suggestions from code review
connorlbark 6d527a7
Update cypress/component/DarCollectionTable/researcher_actions.spec.js
connorlbark abae302
merged with develop
lu-c 207b559
fixed merge issue
lu-c 738971b
???? Testing new actions component.
lu-c 049cbb0
updated unit tests?
lu-c 139bd50
Fixed eslint
lu-c 3e57b6a
Merge branch 'DUOS-1903' of github.com:DataBiosphere/duos-ui into DUO…
lu-c b4ba695
Merge branch 'DUOS-1903' into DUOS-1905
connorlbark 40ddc16
fix name
connorlbark b0038ac
fix admin console
connorlbark 608715a
fix tests
connorlbark dd377b2
Fixing unit tests
lu-c 18b85e0
Apply suggestions from code review
connorlbark 9fe3206
added unit test for Update button (used to be combined with the vote …
lu-c b69adb8
Update cypress/component/DarCollectionTable/admin_actions.spec.js
connorlbark df99fb2
fix codacy
connorlbark 6138a59
Fixed unit tests
lu-c b5d84dc
Fixed unit tests?
lu-c fc96fd7
Merge branch 'DUOS-1902' of github.com:DataBiosphere/duos-ui into DUO…
lu-c 60fea9b
merged with DUOS-1902, deleted unnecessary files
lu-c 7e8e79c
Merge branch 'DUOS-1905' of github.com:DataBiosphere/duos-ui into DUO…
lu-c 3fe5316
Merged with 1905
lu-c aa1bbf2
Fixing unit tests?
lu-c e825982
Fixing eslint
lu-c fb8c1e1
Fixing unit test, but dont think this will sovle the problem
lu-c 7a65a4e
Fixing unit test, but dont think this will sovle the problem
lu-c c5a571f
Fixing unit test, but dont think this will sovle the problem
lu-c 857fdb8
feeling more confident now that the problem will be solved haha
lu-c cd89dca
DUOS-1997 updated open and close collection functions for Admin and C…
JVThomas d022d9d
DUOS-1997 updated integration tests for collection callback functions
JVThomas dc2f0e7
Updated variable names, due to merge conflict discrepancies!
lu-c e692549
Updated variable names, due to merge conflict discrepancies!
lu-c d222213
testing cypress tests on github
lu-c 4cf9270
merged with DUOS-1903
lu-c e5aa265
Checking original researcher unit test. Getting mildly frustrated!
lu-c 7871bc1
Converted hard coded researcher to console type
lu-c e55aeb1
fixing?
lu-c 0f7e151
fix test
connorlbark 635734b
removed unnecessary line
lu-c d997737
re-added actions (sorry connor, i deleted it)
lu-c fc1c330
Merge branch 'DUOS-1904-2' into DUOS-1997
JVThomas b96e81d
DUOS-1997 used USER_ROLES instead of explicit string values
JVThomas 8db6702
DUOS-1997 updated spec to use USER_ROLES instead of hard coded strings
JVThomas 9d834bd
DUOS-1997 resolved merge conflicts
JVThomas 5eb5e35
DUOS-2014 fixed missing forEach immport from lodash, updated actions …
JVThomas 6108db9
DUOS-2014 adjusted tooltip on cancel action, removed unused function,…
JVThomas 03bad5a
DUOS-2014 resolved merge conflicts
JVThomas 85008d0
DUOS-2014 updated iterating function on applyHoverEffects to match no…
JVThomas fe13e7a
DUOS-2014 updated getProjectTitle helper method, updated style update…
JVThomas de0ce7a
DUOS-2014 removed getProjectTitle, inlined collection name in string
JVThomas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you feel okay with leaving these as the original names? It seems less confusing that way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there's two forEach's being used in this file, one from 'lodash/fp' and one from 'lodash'. The reason being is that with the fp version, the function is
forEach(collection, (value) => //code here)
where as in the regular version itsforEach(value, key) => //insert code here)(collection)
.As for why we need two versions, remember the error Shae pointed out here: #1718 (review)
It's being caused by this function in
utils.js
export const applyHoverEffects = (e, style) => { nonFPForEach((key, value) => { e.target.style[key] = value; })(style); };
What had happened was that the normal
forEach
that was used here was replaced with thefp
variant. The difference in the iterating function causes the function to error out.So in short, we need both functions, but they can't be named the same variable, hence the
nonFPForEach
.That said, I still need to update the iterating function to match the non-fp version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! Didnt realize it was because both lodash and lodash/fp were beign used.... That being said, I still think the name is confusing (just because it wasn't immediately obvious to me what it was "nonFP" was supposed to mean. Don't know what to do about that though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it could be
lodashForEach
andlodashFpForEach
? that way they mirror each otherThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way to solve this is to import a prefix for either the non-fp or the fp version. In the past, I've used
import _ from 'lodash';
andimport * as fp from 'lodash/fp';
, for exampleThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but that would cause Webpack to import the entire
lodash
library on file bundling, and lodash is a pretty big library. Even if the code is minified, I'd rather not import methods that are never used for a particular file.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works for me