Feat: add transformation dropdown to histogram #1666
Merged
Conversation
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
pieterlukasse
force-pushed
the
feat/add_transformation_to_histogram
branch
from
2a1fdde to
f688fa4
Compare
...and adjust histogram endpoint call to include histogram concept id in variables list instead of being part of the url, according to the backend changes implemented for this same endpoint.
pieterlukasse
force-pushed
the
feat/add_transformation_to_histogram
branch
from
f688fa4 to
c3d620f
Compare
Please find the detailed integration test report here Please find the ci env pod logs here |
Please find the detailed integration test report here Please find the ci env pod logs here |
...instead, we want the filter just to be recorded as part of the concept item and be reflected in the histogram UI as a line at this point.
Please find the detailed integration test report here Please find the ci env pod logs here |
pieterlukasse
force-pushed
the
feat/add_transformation_to_histogram
branch
from
300af5b to
5a5bc34
Compare
There was a problem hiding this comment.
Great work! I confirmed that I could render the min / max lines and receive different data visualizations with different transformation.
I did a pre-review per your request. I found an ESLINT error and made some code recommendations / nitpicks (feel free to ignore if you don't agree). We might want to break up any new UX requirements into separate tickets if needed. Fixing the accessibility issues found by Axe might not be practical and will need to addressed in the Gen3.2 port.
UX Suggestions:
- Add chart icon to initial screen per design
- Style chart title to be bold
- Format labels for dropdowns to match design
- Implementation missing text version, and text version / Histogram button controls
- Add label for transformation dropdown and place below Cancel / Submit buttons
- Use controlled react components to store state of user selections - I'm seeing a bug where the user inputs disappear after inputting them and changing the transformation type. However, I think the lines associated with the min and max values remain after the chart redrawing.
Notes on Input UX & Error Messaging:
- Currently users can enter non-numeric inputs into the input fields (which then are deleted when leaving focus). Consider adding an error message to let user know only numeric inputs are allowed after user enters an invalid input type.
- Consider adding error messaging for numeric inputs that make the chart render oddly or prevent invalid inputs. For example, if the user adds mins and maximums of -123456789101112131415 and 123456789101112131415 the chart's labels overlap.
- Consider adding error messaging if the min exceeds the max and vice-versa.
- Consider adding error messaging if min exactly equals max
Accessibility Issues:
- In Transformation dropdown: Missing labels for form elements
- In Transformation dropdown: Ensure elements with ARIA roles have all required ARIA attributes
DESIGN
IMPLEMENTATION
FOUND ACCESSIBILITY ISSUES
src/Analysis/GWASApp/Components/Diagrams/PhenotypeHistogram/PhenotypeHistogram.jsx
Outdated
Show resolved
Hide resolved
src/Analysis/GWASApp/Components/Diagrams/PhenotypeHistogram/PhenotypeHistogram.jsx
Outdated
Show resolved
Hide resolved
pieterlukasse
force-pushed
the
feat/add_transformation_to_histogram
branch
from
5a5bc34 to
a613c16
Compare
Please find the detailed integration test report here Please find the ci env pod logs here |
|
Please find the ci env pod logs here |
Please find the detailed integration test report here Please find the ci env pod logs here |
…mes to components and storybooks to better match design
jarvisraymond-uchicago
previously approved these changes
Feb 6, 2025
jarvisraymond-uchicago
approved these changes
Feb 6, 2025
Please find the detailed integration test report here Please find the ci env pod logs here |
pieterlukasse
changed the base branch from
master
to
feat/filters_and_transformation_main
pieterlukasse
merged commit b630445
into
feat/filters_and_transformation_main
8 of 9 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Link to JIRA ticket if there is one: https://ctds-planx.atlassian.net/browse/VADC-1635
New Features