Skip to content

Conversation

@hahahannes
Copy link

@hahahannes hahahannes commented Aug 19, 2025

closes: #65

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign thesuperzapper for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@juliusvonkohout
Copy link
Member

You need to sign your commits to pass the DCO @hahahannes

@juliusvonkohout
Copy link
Member

@andyatmiami @akagami-harsh for review

@andyatmiami
Copy link
Contributor

andyatmiami commented Aug 20, 2025

can/will pivot to this after my review of #115

update: sorry, taking a bit longer to wrap #115 than I anticipated - but I should be able to focus on this PR Tuesday, August 26th... got my environment set up today to reproduce the issue - and its just a matter of updating to confirm fix and actually performing the review. appreciate patience and be in touch soon.

@hahahannes
Copy link
Author

You need to sign your commits to pass the DCO @hahahannes

Done!

@andyatmiami
Copy link
Contributor

@hahahannes - do you mind fixing the PR description to link to #65 ?

I must admit I was very confused when initially trying to get up to speed on the work here - as I couldn't understand how this addressed a11y concerns 😆

@hahahannes
Copy link
Author

@hahahannes - do you mind fixing the PR description to link to #65 ?

I must admit I was very confused when initially trying to get up to speed on the work here - as I couldn't understand how this addressed a11y concerns 😆

I changed, sorry for the confusion!

Copy link
Contributor

@andyatmiami andyatmiami left a comment

Choose a reason for hiding this comment

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

Seems like we will need to do something to adjust failing unit tests as a result of this change:

image

The 2nd failure is easily/obviously correlated to the removal of this.set('routeHash.path... - but probably need a little investigation to understand why the first test failure occurs (could be due to same reason - I haven't verified yet)

@andyatmiami
Copy link
Contributor

So, @hahahannes - could you maybe share with me a custom container image and any related manifests I can load into my local Central Dashboard setup?

I was trying to verify the fix via your comments here:

Maybe I am just not understanding the table - but I don't see any change in behavior when I build a centraldashboard container image and set it in my cluster 🤔

Wanna be able to clearly "see" the bad behavior + your fixed behavior - and struggling to do that right now...

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed if no further activity occurs.
Thank you for your contributions.

Members may comment /lifecycle frozen to prevent this pull request from being marked as stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

iframe synchronization not working

3 participants