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

Tab bar - Add suffix to hidden tab bar 'li' elements #11622

Merged
merged 3 commits into from
Sep 16, 2022

Conversation

akhileshraju
Copy link
Contributor

@akhileshraju akhileshraju commented Aug 31, 2022

Signed-off-by: Akhilesh Raju [email protected]

Issue

Non unique ID values for <li> elements in the hidden and visible tab bar's on the left and right side.
Without this, when I try writing a playwright page model to click the tab bar button using the Playwright Locator, I get a "strict mode" error from Playwright which signifies that it got more than one element for given ID.
AFAIK, element ID's must be unique across the DOM.

https://community.theia-ide.org/t/dom-element-id-field-not-unique-for-tab-bar-entries/2563

What it does

Code changes to add a suffix -hidden to the <li> elements to ensure there is no ID clash.

Before the fix

After the fix

How to test

  • Launch browser/electron app and inspect the elements in the hidden tab bar using the browser DevTools and notice the new -hidden suffix

Review checklist

Reminder for reviewers

@akhileshraju
Copy link
Contributor Author

akhileshraju commented Aug 31, 2022

@msujew @vince-fugnitto if you can, would appreciate your review 🙂

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@akhileshraju thank you for your first contribution, I only have some minor comments 👍

packages/core/src/browser/shell/tab-bars.ts Show resolved Hide resolved
packages/core/src/browser/shell/tab-bars.ts Outdated Show resolved Hide resolved
packages/core/src/browser/shell/tab-bars.ts Outdated Show resolved Hide resolved
@vince-fugnitto vince-fugnitto added the core issues related to the core of the application label Sep 1, 2022
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes work well for me 👍
@msujew do you mind reviewing as well?

packages/core/src/browser/shell/tab-bars.ts Outdated Show resolved Hide resolved
packages/core/src/browser/shell/tab-bars.ts Outdated Show resolved Hide resolved
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

The changes look reasonable to me and don't seem to cause any regressions. Looks good 👍 (after applying the comments by Vince of course)

@vince-fugnitto vince-fugnitto merged commit a982f08 into eclipse-theia:master Sep 16, 2022
@vince-fugnitto vince-fugnitto added this to the 1.30.0 milestone Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core issues related to the core of the application
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants