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(THEEDGE-3764): fixes LoadingCard component #2123

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

ldjebran
Copy link
Contributor

@ldjebran ldjebran commented Dec 14, 2023

A regression happened from latest changes (#1968) when used in federated mode, preventing the model dialogs to open, as no way in federated mode to find the modelId.

This PR revert the change related of using useParams in function "Clickable" and add the full pathname to the url, also needed as in federated mode the path was truncated for example when used in edge-management app the path was "/edge/ipv4", with this change the path is correct and looks like "/edge/inventory/9ea55e32-6e30-4288-bf08-0a877c7fd306/ipv4"

FIXES: https://issues.redhat.com/browse/THEEDGE-3764

fixes-inventory-loading-card-component-2023-12-14 11-23

@ldjebran ldjebran requested a review from a team as a code owner December 14, 2023 10:30
@ldjebran ldjebran force-pushed the fixes-loading-card-component branch from c11ac78 to 29886ed Compare December 14, 2023 10:37
@ldjebran ldjebran changed the title fix(THEEDGE-3764): fixes LoadingCard component. fix(THEEDGE-3764): fixes LoadingCard component Dec 14, 2023
A regression happened from latest changes when used in federated mode, preventing the model dialogs to open, as no way in federated mode to find the modelId. This PR revert the change related of using useParams function "Clickable" and add the full pathname to the url, also needed as in federated mode the path was truncated for example when used in edge-management app the path was "/edge/ipv4", with this change the path is correct and looks like "/edge/inventory/9ea55e32-6e30-4288-bf08-0a877c7fd306/ipv4"
FIXES: https://issues.redhat.com/browse/THEEDGE-3764
@ldjebran ldjebran force-pushed the fixes-loading-card-component branch from 29886ed to 5f400f1 Compare December 14, 2023 11:17
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (aff72b5) 57.51% compared to head (5f400f1) 57.45%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2123      +/-   ##
==========================================
- Coverage   57.51%   57.45%   -0.06%     
==========================================
  Files         193      193              
  Lines        6195     6196       +1     
  Branches     1715     1715              
==========================================
- Hits         3563     3560       -3     
- Misses       2632     2636       +4     

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

@ldjebran
Copy link
Contributor Author

/retest

@gkarat gkarat added the bug Something isn't working label Dec 14, 2023
Copy link
Contributor

@gkarat gkarat left a comment

Choose a reason for hiding this comment

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

Thanks @ldjebran for the PR. I tested it locally and it works fine.

However, I think the reason why edge is having such issues is due to the fact that it hasn't yet migrated to react-router v6, uses its own Router and doesn't set react-router as singleton in it webpack config. This way, by sharing the same router instance across apps, the federated module will also respect the pathname and recognise route params. I recommend to look into this and reserve some time in the soonest future to avoid such bugs and workarounds.

@gkarat gkarat merged commit d9c6f62 into RedHatInsights:master Dec 14, 2023
2 checks passed
@gkarat
Copy link
Contributor

gkarat commented Dec 14, 2023

🎉 This PR is included in version 1.61.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
bug Something isn't working released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants