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

fix: invalid item in dropdown #809

Merged
merged 7 commits into from
Aug 11, 2023
Merged

fix: invalid item in dropdown #809

merged 7 commits into from
Aug 11, 2023

Conversation

4nalog
Copy link
Member

@4nalog 4nalog commented Aug 1, 2023

Read then delete this section

- Make sure to use a concise title for the pull-request.

- Use #patch, #minor or #major in the pull-request title to bump the corresponding version. Otherwise, the patch version
will be bumped. More details

TL;DR

Please replace this text with a description of what this PR accomplishes.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue

fixes https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

@4nalog 4nalog requested a review from FrankFlitton August 1, 2023 16:51
@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #809 (4baf041) into master (184cd75) will decrease coverage by 0.01%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #809      +/-   ##
==========================================
- Coverage   63.39%   63.38%   -0.01%     
==========================================
  Files         527      527              
  Lines       13391    13393       +2     
  Branches     2548     2548              
==========================================
  Hits         8489     8489              
- Misses       4902     4904       +2     
Files Changed Coverage Δ
...nents/Breadcrumbs/components/BreadcrumbPopover.tsx 9.67% <33.33%> (-0.22%) ⬇️

Signed-off-by: Frank Flitton <[email protected]>
FrankFlitton
FrankFlitton previously approved these changes Aug 1, 2023

const {
isLoading,
error,
data: popoverQueryData,
} = useQuery(
`breadcrumb-list-${props.id}`,
`breadcrumb-list-${props.id}-${props.value}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should fetch the project list every time the domain changes. I think we should separate the api call and the url generation step so we don't make unnecessary network calls

Copy link
Contributor

Choose a reason for hiding this comment

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

This would be a good enhancement, we'd probably need a query hash key, the formatter and data as separate params in the Breadcrumb objects. Some breadcrumbs don't change from url to url etc...

A note here is that it only fetches once the user opens that dropdown.

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 how the original code worked, not an enhancement. If we separate the list request from the static url generation this wouldn't be a problem


const {
isLoading,
error,
data: popoverQueryData,
} = useQuery(
`breadcrumb-list-${props.id}`,
`breadcrumb-list-${props.id}-${props.value}`,
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 how the original code worked, not an enhancement. If we separate the list request from the static url generation this wouldn't be a problem

ursucarina
ursucarina previously approved these changes Aug 8, 2023
Signed-off-by: Carina Ursu <[email protected]>
@ursucarina ursucarina dismissed stale reviews from FrankFlitton and themself via 9e73249 August 8, 2023 19:08
ursucarina
ursucarina previously approved these changes Aug 9, 2023
Signed-off-by: Carina Ursu <[email protected]>
ursucarina
ursucarina previously approved these changes Aug 9, 2023
@FrankFlitton FrankFlitton merged commit f569aa5 into master Aug 11, 2023
@FrankFlitton FrankFlitton deleted the sohamparekh/issue-4403 branch August 11, 2023 17:32
@flyte-bot
Copy link
Collaborator

🎉 This PR is included in version 1.9.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants