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

Add New Line Lists with Medium Information #1656

Merged
merged 11 commits into from
Sep 26, 2022

Conversation

duytnguyendtn
Copy link
Collaborator

@duytnguyendtn duytnguyendtn commented Sep 16, 2022

Description

@camipacifici and I have been investigating the medium information for the line lists, and it has evolved into a much more difficult task, between trying to track down the original source material, resolving dead links, contacting old colleagues, etc. Some of those (easier ones) were put into #1626. Some additional ones (possibly more controversial) are here. I've officially run out of my timebox on this ticket, so I wanted to close out my initial findings and commit them now.

This PR makes two changes to our existing line lists:

  1. The Atomic/Ionic list seems to be a combination two separate lists. We were able to track down the original source material, for the majority of it, which contained the medium info. The remaining portion, however, eludes me. This PR splits the existing Atomic/Ionic list into two separate lists Atomic/Ionic and Atomic/Ionic Fine Structure, the latter of which we know the medium information for.
  2. To address @rosteen's discovery in Line List: Display medium information from metadata #1626 (review), this PR also adds the Common Galactic list, which largely contains most of the lines from the Common Stellar (not all of them), but split into two wavelength ranges based on the medium.

To support the above changes, the following adjustments were made to the UI to support these longer list names:

  1. Apparently the v-select we were using for the dropdown has a "whitespace buffer" that v-combobox doesn't have. I switched it out to display more of the text name in the dropdown with the same real-estate Unfortunately after further testing, the distinction between the two components is that the v-combobox is a v-autocomplete component; effectively it allows a user to type in anything. With that in mind, a v-select is more appropriate here. That being said, a user can still see the list selected by opening the dropdown.
    image
    I think this was a nice-to-have, so it's not a show stopper that we can't get this in. I feel like @rosteen's review is more focused on the point below, but can correct me if I'm wrong
  2. I removed our overflow restriction on the actual loaded list expansion-panel to show the full list name:

image

Fixes #1517

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.

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?
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@codecov
Copy link

codecov bot commented Sep 16, 2022

Codecov Report

Base: 86.76% // Head: 86.76% // No change to project coverage 👍

Coverage data is based on head (bfa270f) compared to base (3937aa0).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1656   +/-   ##
=======================================
  Coverage   86.76%   86.76%           
=======================================
  Files          95       95           
  Lines        9652     9652           
=======================================
  Hits         8375     8375           
  Misses       1277     1277           
Impacted Files Coverage Δ
...igs/specviz/plugins/line_analysis/line_analysis.py 98.03% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@duytnguyendtn duytnguyendtn marked this pull request as ready for review September 19, 2022 13:35
Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

Thanks for getting a couple more of these in. PR needs to be rebased to main to resolve conflicts. Two other comments:

  1. It looks like Atomic/Ionic isn't available in the preset menu, only Atomic/Ionic Fine Structute, I'm not sure why.
  2. Now that some of the list names are longer than they used to be, it's probably worth having the text in the preset menu wrap so the wavelengths aren't cut off in the menu at the default plugin width.

@duytnguyendtn
Copy link
Collaborator Author

duytnguyendtn commented Sep 19, 2022

It looks like Atomic/Ionic isn't available in the preset menu

Yep, that's intended. Referring back to the description, the Atomic/Ionic list only contains the lines I wasn't able to track down. The The A/I Fine Structure one contains the lines I was able to confirm

Good point about the wrapping. I'll look into it

@rosteen
Copy link
Collaborator

rosteen commented Sep 19, 2022

Yep, that's intended. Referring back to the description, the Atomic/Ionic list only contains the lines I wasn't able to track

Ahh right, got it. Somehow forgot that between reading the description and testing the plugin. Thought it was an air/vac split.

@duytnguyendtn
Copy link
Collaborator Author

@rosteen I've adjusted the UI, let me know if that works for you!

@duytnguyendtn
Copy link
Collaborator Author

The docs build failure seems to be unrelated (didn't change the docs); please ignore!

@rosteen
Copy link
Collaborator

rosteen commented Sep 22, 2022

Looks better to me. This is also present on main, but is there an easy way to change the preset lists menu to wrap text there as well so it doesn't overflow the bounds of the app? Seems like the default of v-select items is not to wrap.

@duytnguyendtn
Copy link
Collaborator Author

duytnguyendtn commented Sep 22, 2022

@rosteen I looked into this, and I unfortunately couldn't find anything. There isn't anything in the v-select docs that did the trick (https://vuetifyjs.com/en/api/v-select) and the closest I could find is this github feature request for the same thing, but for ComboBoxes: vuetifyjs/vuetify#6534. They even provided a working demo. Maybe @mariobuikhuizen or @kecnry has any other ideas?

I think I misread Ricky's intent. Ignore this message

@kecnry
Copy link
Member

kecnry commented Sep 22, 2022

but is there an easy way to change the preset lists menu to wrap text there as well so it doesn't overflow the bounds of the app

I thought I tested this PR yesterday and didn't see any overflow on my machine with the default plugin width, but I would think we could just enable it with css if necessary (or maybe having ellipsis would be better than wrapping?)

@duytnguyendtn
Copy link
Collaborator Author

I think I realized I misunderstood Ricky's earlier message. You can see in my screenshot that there is an overflow of the app's bounds when the menu is dropped down. I wonder if I can anchor the menu to the right, rather than the left...

@rosteen
Copy link
Collaborator

rosteen commented Sep 22, 2022

Sorry, I should have included a screenshot. This is what it looks like for me without messing with anything. Not a huge deal, I don't really think it should block merging this but if it's quick it could be included (sounds like it's not).

Screen Shot 2022-09-22 at 3 36 45 PM

@duytnguyendtn
Copy link
Collaborator Author

Haha I think I got it! @rosteen does this work for you?
image

Copy link
Collaborator

@rosteen rosteen 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 to me 👍

@duytnguyendtn duytnguyendtn mentioned this pull request Sep 22, 2022
9 tasks
@@ -94,6 +94,7 @@
<j-plugin-section-header>Preset Line Lists</j-plugin-section-header>
<v-row>
<v-select
:menu-props="{ left: true }"
Copy link
Member

Choose a reason for hiding this comment

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

we should probably do this app-wide... how does this work in conjunction with #1673?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point @kecnry! I've implemented it across all v-selects in bfa270f

The short answer to your question about how they interact together is that they don't. I WISH they did. Unfortunately #1673 restricts menus to the size of the v-select when you attach. We'll have to think through this case before we merge #1673. I'd advocate for us to get this in as it stands and think about those considerations over in that PR

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.

LGTM (probably could either use a squash & merge or pre-squashing some of the smaller commits - I'll let you decide)!

@duytnguyendtn
Copy link
Collaborator Author

Thanks @kecnry and @rosteen for the eyes! I'll squash and merge (my new default)

@duytnguyendtn duytnguyendtn merged commit 1bd57b7 into spacetelescope:main Sep 26, 2022
@duytnguyendtn duytnguyendtn deleted the newlists branch September 26, 2022 13:26
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.

[FEAT] Specviz: Add air/vacuum to line list metadata and display in plugin
3 participants