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

Add URL string union hook to better integrate url params with Tab components #5111

Merged
merged 2 commits into from
Mar 9, 2023

Conversation

dvail
Copy link
Contributor

@dvail dvail commented Mar 6, 2023

Description

This adds a new hook that simplifies using a string union safely with a URL parameter. This should handle any case where we have a finite list of strings that are valid as a URL parameter, such as the current use case of having a known set of PF Tab components that we want to integrate with the URL.

Even though this is for VM 2.0 right now, I'll add the @stackrox/ui team for awareness in case this is useful elsewhere.

This was actually a net removal of code, before comments and tests...

Checklist

  • Investigated and inspected CI test results
  • Unit test and regression tests added
  • Evaluated and added CHANGELOG entry if required
  • Determined and documented upgrade steps
  • Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

Unit tests.

Cycle through tab clicks and ensure the correct tab is highlighted and that the URL parameter changes accordingly.
image
image
image

Visit the page via direct navigation with a non-default tab parameter selected and verify that the correct tab is selected.

Visit the page via direct navigation with an invalid parameter value and verify that the default tab is selected and parameter is changed to the default.

@openshift-ci
Copy link

openshift-ci bot commented Mar 6, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@roxbot
Copy link
Contributor

roxbot commented Mar 6, 2023

Images are ready for the commit at caa4d47.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.74.x-326-gcaa4d478eb.

@dvail dvail force-pushed the dv/ROX-15384-implement-image-single-page-summary branch from 81b074b to 16bf636 Compare March 8, 2023 18:28
Base automatically changed from dv/ROX-15384-implement-image-single-page-summary to master March 8, 2023 20:19
@dvail dvail requested review from alwayshooin and a team March 8, 2023 20:55
@dvail dvail marked this pull request as ready for review March 8, 2023 20:56
expect(initialValidResult.current[0]).toBe('Beta');
expect(params.get('urlKey')).toBe('Beta');

const { result: initialInvalidResult } = renderHook(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the start of a separate test?

  • See comment above: if the URL param already contains a valid value
  • See comment below: when the URL param is invalid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be: 'should use the default value when an invalid value is entered directly into the URL'

Copy link
Contributor

@pedrottimark pedrottimark left a comment

Choose a reason for hiding this comment

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

Wow, you keep teaching me more TypeScript!

One comment about unit tests.

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.

None yet

3 participants