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

Enable Row Selection on Plugin Table Component #2856

Merged
merged 3 commits into from
May 7, 2024

Conversation

duytnguyendtn
Copy link
Collaborator

@duytnguyendtn duytnguyendtn commented May 6, 2024

Description

Hello Jdaviz Team!

This PR is a small change to the Plugin Table component to expose the Vuetify row selection attributes for the Virtual Observatory plugin I'm working on. Because this is a frontend component, no new tests were added. A changelog entry will follow the publication of this PR, once a PR number is assigned.

image

It was great to sync my fork again :)
Screenshot 2024-05-06 150559

Change log entry

  • Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
    list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
    should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

@duytnguyendtn duytnguyendtn added this to the 3.11 milestone May 6, 2024
@pllim pllim added feature Feature request plugin Label for plugins common to multiple configurations labels May 6, 2024
@pllim
Copy link
Contributor

pllim commented May 6, 2024

But how do you grab the selected rows for further operations? Is a test possible for this?

Thanks!

@duytnguyendtn
Copy link
Collaborator Author

duytnguyendtn commented May 6, 2024

You can see an example of its intended usage in my VO plugin here: https://github.com/duytnguyendtn/SkyviewJdaviz/blob/pyvo/pyvo_plugin/pyvo.py#L128-L141, but in short, you access it via the selected_rows attribute on the table via:

user_selection = my_plugin.table.selected_rows

The only programmatic way I know of selecting the rows is by setting selected_rows directly. In which case, the test would just be setting, then immediately reading the same traitlet, which didn't seem like a useful test to me 😅. Maybe @kecnry can weigh in with some front-end wizardry?

Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

Looks simple enough to me - thanks! I'm not really sure there is a way to test this until there is an actual use-case either. If we're worried we may not have a use-case, we could let this sit unmerged until there is one and then add test coverage there 🤷‍♂️

Comment on lines +85 to +86
- Plugin Table components now support row selection. [#2856]

Copy link
Member

Choose a reason for hiding this comment

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

since this isn't actually used anywhere, it might not need a changelog entry?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Entirely up to y'all, just need the instruction and I can remove it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Since people can use this downstream, let's keep it.

@pllim
Copy link
Contributor

pllim commented May 7, 2024

we could let this sit unmerged

My understanding is that Duy would use this downstream, no?

@duytnguyendtn
Copy link
Collaborator Author

we could let this sit unmerged

Duy would use this downstream, no?

Yes, I'll be using this downstream. The Virtual Observatory plugin will be it's own PR, but I could build that PR on top of this one. I mainly tried to break up the effort to make it easier for reviews. Entirely up to y'all how you'd like to proceed.

@kecnry
Copy link
Member

kecnry commented May 7, 2024

I have no objection to merging myself - this is pretty isolated and would be pretty easy to strip back out if for some reason it goes unused (which seems unlikely). @duytnguyendtn - can you please make a note to try to add test coverage for this capability in the eventual PR that makes use of it?

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Kyle, feel free to merge when that extra comment you asked for is added. Thanks, all!

Comment on lines +85 to +86
- Plugin Table components now support row selection. [#2856]

Copy link
Contributor

Choose a reason for hiding this comment

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

Since people can use this downstream, let's keep it.

Comment on lines +4365 to +4366
# NOTE: These UI features are not covered in test coverage. Plugins making use of
# this feature should ensure test coverage for their respective tables.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a note regarding test coverage here. Thanks!

Copy link

codecov bot commented May 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.71%. Comparing base (71c8197) to head (c6e12d4).
Report is 18 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2856      +/-   ##
==========================================
- Coverage   88.90%   88.71%   -0.20%     
==========================================
  Files         111      111              
  Lines       17148    17113      -35     
==========================================
- Hits        15246    15181      -65     
- Misses       1902     1932      +30     

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

@kecnry kecnry merged commit e73f607 into spacetelescope:main May 7, 2024
17 of 18 checks passed
@duytnguyendtn duytnguyendtn deleted the guess_whos_back branch May 7, 2024 19:37
duytnguyendtn added a commit to duytnguyendtn/jdaviz that referenced this pull request Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request plugin Label for plugins common to multiple configurations Ready for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants