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(KFLUXUI-177): create NamespaceProvider to use activeNamespace #100

Merged
merged 8 commits into from
Feb 6, 2025

Conversation

sahil143
Copy link
Collaborator

Fixes

https://issues.redhat.com/browse/KFLUXUI-177

Description

Route Definition Utility Implementation

This PR introduces a centralized route configuration system using the buildRoute utility, which provides type-safe path construction and nested route capabilities. The utility enables:

1. Path Construction with parameters

// Creates base path with parameter replacement capability
export const WORKSPACE_PATH = buildRoute(`workspaces/:${RouterParams.workspaceName}`); // `workspaces/:workspaceName`
// Usage with parameters:
// WORKSPACE_PATH.createPath({ workspaceName: "my-workspace" }) // workspace/my-workspace

2. Nested Route Extension through chaining

// Extend base paths with additional segments
export const IMPORT_PATH = WORKSPACE_PATH.extend('import'); // `workspaces/:workspaceName/import
export const APPLICATION_LIST_PATH = WORKSPACE_PATH.extend(`applications`); // `workspaces/:workspaceName/applications

3. Type-Safe Parameters enforcement

// All routes are aggregated in paths.ts for single-source imports
export { WORKSPACE_PATH, IMPORT_PATH } from './page-routes/workspace';

NamespaceProvider

use useNamespace instead of useWorkspaceInfo to get the active namespace.

Type of change

  • Feature
  • Bugfix
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Screen shots / Gifs for design review

How to test or reproduce?

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@CryptoRodeo
Copy link
Contributor

I have not had time to run this locally, but this is looking really good so far.
Like @abhinandan13jan , I am also curious on how query params would work with the new RouteDefinition type.

Could it look something like this?:

<Link {...props} to={IMPORT_PATH.createPath({ workspaceName: namespace }) + '?example=value'} />

@sahil143
Copy link
Collaborator Author

sahil143 commented Feb 3, 2025

I have not had time to run this locally, but this is looking really good so far. Like @abhinandan13jan , I am also curious on how query params would work with the new RouteDefinition type.

Could it look something like this?:

<Link {...props} to={IMPORT_PATH.createPath({ workspaceName: namespace }) + '?example=value'} />

Yes, returned string from createPath can be concatenated with other strings, We can also have utilities to build queryParams strings on need basis.

Copy link

codecov bot commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 81.56425% with 33 lines in your changes missing coverage. Please review.

Project coverage is 80.12%. Comparing base (2c479bd) to head (0bf03bf).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
src/components/Namespace/utils.ts 48.48% 17 Missing ⚠️
src/components/Namespace/namespace-context.tsx 92.85% 4 Missing ⚠️
src/components/Namespace/index.ts 0.00% 3 Missing ⚠️
src/routes/utils.ts 90.90% 2 Missing ⚠️
...ents/Applications/switcher/ApplicationSwitcher.tsx 90.00% 1 Missing ⚠️
src/components/Overview/IntroBanner.tsx 0.00% 1 Missing ⚠️
src/models/namespace.ts 80.00% 1 Missing ⚠️
src/routes/index.tsx 0.00% 1 Missing ⚠️
src/routes/page-routes/application.tsx 0.00% 1 Missing ⚠️
src/routes/page-routes/workspace.tsx 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #100      +/-   ##
==========================================
- Coverage   80.13%   80.12%   -0.02%     
==========================================
  Files         570      578       +8     
  Lines       21529    21554      +25     
  Branches     5076     5348     +272     
==========================================
+ Hits        17253    17270      +17     
- Misses       4251     4258       +7     
- Partials       25       26       +1     
Flag Coverage Δ
unittests 80.12% <81.56%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@JoaoPedroPP
Copy link
Contributor

I noticed the use of useWorkspaceInfo() is still in place in src/components/Applications/ApplicationListRow.tsx . Should we keep or remove it ?

@sahil143
Copy link
Collaborator Author

sahil143 commented Feb 6, 2025

I noticed the use of useWorkspaceInfo() is still in place in src/components/Applications/ApplicationListRow.tsx . Should we keep or remove it ?

Any usage of useWorkspaceInfo() should be removed.

@sahil143 sahil143 merged commit c6528e9 into konflux-ci:main Feb 6, 2025
4 of 5 checks passed
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.

4 participants