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

frontend: Hide back button with null backLink #2034

Merged
merged 1 commit into from
Jun 14, 2024
Merged

Conversation

skoeva
Copy link
Contributor

@skoeva skoeva commented Jun 10, 2024

This fix explicitly handles the null case in getBackLink() to ensure that no back button will be shown, as defined in MainInfoSectionProps.

Fixes: #2033

Testing

  • Go to frontend/src/components/crd/Details.tsx and set backLink to null in the MainInfoSection component
  • Open a cluster in Headlamp (npm start) and navigate to the Custom Resources page
  • Click on a CRD and ensure that the back button does not appear on the CRD page

@skoeva skoeva linked an issue Jun 10, 2024 that may be closed by this pull request
@skoeva skoeva self-assigned this Jun 11, 2024
@skoeva skoeva force-pushed the backlink-fix branch 2 times, most recently from bb8c6d3 to 6834205 Compare June 12, 2024 02:35
@skoeva skoeva marked this pull request as ready for review June 12, 2024 03:16
@illume
Copy link
Collaborator

illume commented Jun 12, 2024

I think instead just not adding the backLink would work for this(or backLink=false). If no backLink is supplied, then no back link is used.

Can you please explain a bit more why this is necessary?

@skoeva
Copy link
Contributor Author

skoeva commented Jun 12, 2024

I think instead just not adding the backLink would work for this(or backLink=false). If no backLink is supplied, then no back link is used.

Can you please explain a bit more why this is necessary?

@illume The backLink value is by default the empty string '', and the spec for backLink specifies a difference between passing in the empty string and a null value:

/** The route or location to go to. If it's an empty string, then the "browser back" function is used. If null, no back button will be shown. */

Previously passing in a null value did the same thing as passing in the empty string. Unless this is the intended behavior, we are missing the functionality to handle the null string as specified

@joaquimrocha
Copy link
Collaborator

Yeah. By default we have had the backLink, so we should just fix the null case which is what docs mean. The backLink={false} should also be the same as null.

Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

My comment was in the context of MainInfoSection. You need to account for null there, but checking SectionBox, where it is then used, we shouldn't update it to receive null but instead just pass false to it from MainInfoSection as that's what null means.

@skoeva skoeva force-pushed the backlink-fix branch 3 times, most recently from 38be0e6 to 027f445 Compare June 14, 2024 14:20
@skoeva skoeva closed this Jun 14, 2024
@skoeva
Copy link
Contributor Author

skoeva commented Jun 14, 2024

Ran into merge conflict issues...reopening here #2040

@joaquimrocha
Copy link
Collaborator

We shouldn't close PRs because of merge conflicts. I am reopening this and closing the new one since this one has the context from previous reviews.

@joaquimrocha joaquimrocha reopened this Jun 14, 2024
@skoeva skoeva force-pushed the backlink-fix branch 2 times, most recently from 35d1827 to 1c6d7a6 Compare June 14, 2024 16:16
@joaquimrocha joaquimrocha self-requested a review June 14, 2024 17:43
@vyncent-t
Copy link
Contributor

vyncent-t commented Jun 14, 2024

this is a nit but the MainInfoSection part could be left off to make it frontend: Fixes handle null backLink

careful that when changing the commit message it still works

@skoeva skoeva changed the title frontend: MainInfoSection: Correctly handle null backLink frontend: Hide back button with null backLink Jun 14, 2024
This fix explicitly handles the null case in getBackLink() to ensure
that no back button will be shown, as defined in MainInfoSectionProps.

Fixes: #2033

Signed-off-by: Evangelos Skopelitis <eskopelitis@microsoft.com>
@joaquimrocha
Copy link
Collaborator

this is a nit but the MainInfoSection part could be left off to make it frontend: Fixes handle null backLink

Why? I think the only thing not according to format is the first :. Normally we do frontend MainInfoSection: MESSAGE

@joaquimrocha joaquimrocha merged commit d97753d into main Jun 14, 2024
16 checks passed
@joaquimrocha joaquimrocha deleted the backlink-fix branch June 14, 2024 21:15
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.

Setting BackLink to null does not work
4 participants