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

Update/populateinstruments #106

Merged
merged 7 commits into from
Dec 16, 2024
Merged

Update/populateinstruments #106

merged 7 commits into from
Dec 16, 2024

Conversation

capetillo
Copy link
Collaborator

UPDATE: Populate instruments utils function

Background:
I was making two separate requests to two separate apis to get the instruments list for two separate components. Now it's a utils function and makes the request to the instruments endpoint of the observation portal.

Implementation:
Same as before, go through each value of the response and filter for only 0m4 class that have filters and are schedulable.

VISUALS:

Screenshot of filter list populating like before

Screenshot 2024-12-11 at 1 15 45 PM Screenshot 2024-12-11 at 2 00 29 PM

Copy link
Contributor

@mgdaily mgdaily left a comment

Choose a reason for hiding this comment

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

Mostly looks good, but a comment about telescope class hardcoding

})
const filterList = []
Object.values(response).forEach((instrument) => {
if (instrument.class === '0m4' && instrument.optical_elements.filters) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is probably fine for now, but if Wayne's telescopes are integrated they may be 0m35 or something different, and we generally would want to keep this easily adjustable. You could put it in a global at the top of the file, or read it from the public/config/config.json which we can set in the deployment at runtime!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oo I like that!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm intrigued by this read it from the public/config/config.json which we can set in the deployment at runtime. I'll be declaring it as a global config and ask you about that statement in person (or you can also explain here but I'm patient and willing to rewrite this when the time comes)

@capetillo capetillo requested a review from mgdaily December 11, 2024 22:42
Copy link
Collaborator

@LTDakin LTDakin left a comment

Choose a reason for hiding this comment

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

LGTM

@capetillo capetillo merged commit 1072570 into main Dec 16, 2024
1 check passed
@capetillo capetillo deleted the update/populateinstruments branch December 16, 2024 18:30
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.

3 participants