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

feat(ui): freight history in stage details #2730

Merged
merged 5 commits into from
Oct 14, 2024
Merged

Conversation

Marvin9
Copy link
Contributor

@Marvin9 Marvin9 commented Oct 11, 2024

Screenshot 2024-10-11 at 4 21 45 PM (2)

Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
@Marvin9 Marvin9 requested a review from a team as a code owner October 11, 2024 19:30
Copy link

netlify bot commented Oct 11, 2024

Deploy Preview for docs-kargo-io ready!

Name Link
🔨 Latest commit db972eb
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-io/deploys/67098f6c1ffd410008655e2b
😎 Deploy Preview https://deploy-preview-2730--docs-kargo-io.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

const queryClient = useQueryClient();
const navigate = useNavigate();

const { freightHistoryPerWarehouse, freightMap } = useMemo(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rpelczar I have moved this to useMemo

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.16%. Comparing base (2bb71f2) to head (db972eb).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2730      +/-   ##
==========================================
- Coverage   49.16%   49.16%   -0.01%     
==========================================
  Files         270      270              
  Lines       19356    19363       +7     
==========================================
+ Hits         9516     9519       +3     
- Misses       9218     9221       +3     
- Partials      622      623       +1     

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

}
}

const freightData = queryClient.getQueryData(
Copy link
Contributor Author

@Marvin9 Marvin9 Oct 11, 2024

Choose a reason for hiding this comment

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

@rpelczar

Why not regular fetch? I think you assumed there are data. Maybe it will work always in our case, but not sure if it is a good practice. You can set a stale property for data in query configuration, so it won't generate a new request. Also, you are sure that if the data are not available, it will be fetched. Let me know what you think.

You are right, this will work in our case. I didn't want to make the query for the data we already have and I wanted to avoid adding context provider or prop drilling to have that data. The closest I can do is ensureQueryData but since its async I have to wrap in effect. I will look whether to make query with invalidation or use ensureQueryData and add state

Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
@krancour
Copy link
Member

Test drove and this worked quite nicely. 🔥

LGTM as long as @rpelczar feels all his feedback was adequately addressed.

@Marvin9 Marvin9 requested a review from rpelczar October 14, 2024 12:28
Copy link
Contributor

@rpelczar rpelczar left a comment

Choose a reason for hiding this comment

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

LGTM

@Marvin9 Marvin9 added this pull request to the merge queue Oct 14, 2024
Merged via the queue into main with commit a017329 Oct 14, 2024
23 checks passed
@Marvin9 Marvin9 deleted the Marvin9/freight-history-2 branch October 14, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants