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

[ENH] Implemented landing view #37

Merged
merged 6 commits into from
Feb 14, 2025
Merged

[ENH] Implemented landing view #37

merged 6 commits into from
Feb 14, 2025

Conversation

rmanaem
Copy link
Contributor

@rmanaem rmanaem commented Feb 13, 2025

Checklist

This section is for the PR reviewer

  • PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF], [MODEL], [DOC]) (see our Contributing Guidelines for more info)
  • PR has a label for the release changelog or skip-release (to be applied by maintainers only)
  • PR links to GitHub issue with mention Closes #XXXX
  • Tests pass
  • Checks pass

For new features:

  • Tests have been added

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

@rmanaem rmanaem added the pr-patch Incremental feature improvement, will increment patch version when merged (0.0.+1) label Feb 13, 2025
Copy link

codecov bot commented Feb 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.09%. Comparing base (dacd788) to head (d71c113).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #37   +/-   ##
=======================================
  Coverage   88.09%   88.09%           
=======================================
  Files          12       12           
  Lines          42       42           
  Branches        7        7           
=======================================
  Hits           37       37           
  Misses          5        5           
Flag Coverage Δ
tests 88.09% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rmanaem rmanaem requested a review from surchs February 13, 2025 19:06
Copy link

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Looks good @rmanaem . Not sure about the cypress selectors, but otherwise 🧑‍🍳

@@ -15,7 +15,7 @@ function NavigationButton({
};

return (
<Button variant="contained" onClick={handleClick}>
<Button data-cy={`${label.toLowerCase()}-button`} variant="contained" onClick={handleClick}>
Copy link

Choose a reason for hiding this comment

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

that might be a bit risky as a cypress selector, no? What if we change the label, should that break the tests? Not sure what the best solution here is. Maybe numeric identifiers for the views you can navigate to? That might change less frequently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm what would that look like?

Copy link

Choose a reason for hiding this comment

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

Good question. I guess the goal would be to have persistent and unique identifiers for each page/view you can navigate to. Numeric identifiers are easy to make unique and persistent, but they are hard to read and debug. So maybe just a string identifier like column_annotation and multi_column_annotation and so on. Presumably such a list would have to live in the central store of the app.

But maybe let's discuss this first in an issue and then make a decision and implement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, let's do that cause I'm not sure if it's a good idea to keep that in the store. Could you please open the issue?

@rmanaem rmanaem merged commit 3760165 into main Feb 14, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-patch Incremental feature improvement, will increment patch version when merged (0.0.+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

welcome/landing page
2 participants