Skip to content

frontend: Sidebar: Fix conditional expansion description text #3277

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

frontend: Sidebar: Fix conditional expansion description text #3277

wants to merge 2 commits into from
+6 −16

Conversation

Pratik-050
Copy link

Note for reviewers:

Changed the description of the left chevron and right chevron button in the sidebar.

fixes: #3276

image
image

Renamed Collapse sidebar to Shrink sidebar as the i18n support for Collapse Sidebar was there with a block 'S' but not the same for Expand Sidebar. Attached a screenshot of the translation.json for more context:
image
This is why some tests expecting Collapse Sidebar are failing as well.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 18, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Pratik-050
Once this PR has been reviewed and has the lgtm label, please assign sniok for approval. For more information see the 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

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 18, 2025
@illume
Copy link
Contributor

illume commented May 18, 2025

Very cool. Thank you.

You can update the snapshot test with:
npm run test -- -u

We follow a linux kernel style of commit messages. With the context at the front followed by an action word and the rest.
Would you mind changing them to something like this?
frontend: Sidebar: Fix conditional expansion description text

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 18, 2025
@Pratik-050
Copy link
Author

Thanks @illume I've updated the commit message and the tests as well. Thank you for your guidance!

@Pratik-050 Pratik-050 changed the title frontend: app: fix: Add Expand Sidebar conditionally frontend: Sidebar: Fix conditional expansion description text May 18, 2025
@illume
Copy link
Contributor

illume commented May 19, 2025

The check is failing...

Can you please run make frontend-i18n-check in the root of the repo and see what the problem is?

I see we already have t('Shrink sidebar') : t('Expand sidebar') in HeadlampButton.tsx... and in the translation.json files.

eyeaadil
eyeaadil reviewed May 19, 2025
View reviewed changes
Copy link

@eyeaadil eyeaadil left a comment

Choose a reason for hiding this comment

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

Looking Good

@Pratik-050
Copy link
Author

Hello @illume yes we have t('Shrink sidebar') : t('Expand sidebar') in HeadlampButton.tsx but that only changes a aria-label, I think that is for test purpose only.
This error is coming when I'm trying to run make make frontend-i18n-check
Screenshot from 2025-05-19 18-08-45

@illume
Copy link
Contributor

illume commented May 19, 2025

After you run that command, then run

git status

… to see what files it has changed.

it might also be a good idea to git rebase against the latest main branch.

@Pratik-050
Copy link
Author

I took a pull from the latest main just now and ran this command but no file changes are there. @illume

@illume
Copy link
Contributor

illume commented May 19, 2025

Hrmm. Ok. Let me have a closer look in a little bit. If I find a fix I’ll push it to your branch.

@Pratik-050
Copy link
Author

Thanks! @illume

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

@eyeaadil eyeaadil eyeaadil left review comments

@illume illume Awaiting requested review from illume

@joaquimrocha joaquimrocha Awaiting requested review from joaquimrocha

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sidebar expansion description doesn't update with state
4 participants
@Pratik-050 @k8s-ci-robot @illume @eyeaadil