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

Devan/eng 1247 artifactsartifactid #17053

Merged
merged 3 commits into from
Feb 12, 2025
Merged

Conversation

devangrose
Copy link
Contributor

@devangrose devangrose commented Feb 7, 2025

In draft until dependent is merged

Add detail page for each type of data

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • If this pull request removes docs files, it includes redirect settings in mint.json.
  • If this pull request adds functions or classes, it includes helpful docstrings.

@github-actions github-actions bot added the ui-replatform Related to the React UI rewrite label Feb 7, 2025
@devangrose devangrose force-pushed the devan/eng-1247-artifactsartifactid branch 2 times, most recently from 1fb04d7 to 0d58d50 Compare February 10, 2025 16:42
@devangrose devangrose marked this pull request as ready for review February 10, 2025 16:43
@devinvillarosa
Copy link
Contributor

Gut feeling, it seems a bit odd that we need to do artifact.data as string to override the typings in the UI layer.
If artifact.data is expected (even with wrongful typing from openAPI), can't we just add an assert expecting this type to be a string in the api / hook layer?

Comment on lines 28 to 30
if (!artifact) {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think TS thinks artifact might not be truthy because of typing from getQueryService.
On the hook layer, can you assert that res.data is truthy

@devangrose
Copy link
Contributor Author

Gut feeling, it seems a bit odd that we need to do artifact.data as string to override the typings in the UI layer. If artifact.data is expected (even with wrongful typing from openAPI), can't we just add an assert expecting this type to be a string in the api / hook layer?

Smart - I'll put that in

@devangrose
Copy link
Contributor Author

Another reply: we use number values for progress artifacts - which kind of leads me to leave typecasting in the UI

@devinvillarosa
Copy link
Contributor

Another reply: we use number values for progress artifacts - which kind of leads me to leave typecasting in the UI

hmm, would it make sense to have the component to just accept both number and string types then?

@devangrose devangrose force-pushed the devan/eng-1247-artifactsartifactid branch from 5c6148e to ff7dd66 Compare February 10, 2025 19:30
@devangrose
Copy link
Contributor Author

I think my commit here fixed most of it: ff7dd66

I think that we should keep the separate number and string types, I get a little nervous when directly recasting primitives from the backend. I would be up for seeing if the number can be cast as a string on the backend though and then unifying the type

@devangrose devangrose force-pushed the devan/eng-1247-artifactsartifactid branch from ff7dd66 to 1fba2f0 Compare February 11, 2025 17:09
Copy link
Contributor

@devinvillarosa devinvillarosa left a comment

Choose a reason for hiding this comment

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

LGTM.

Just one minor suggestion, and a nit comment

@devangrose devangrose merged commit d208371 into main Feb 12, 2025
9 checks passed
@devangrose devangrose deleted the devan/eng-1247-artifactsartifactid branch February 12, 2025 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui-replatform Related to the React UI rewrite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants