-
Notifications
You must be signed in to change notification settings - Fork 273
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
fix: adding stable selectors for tab container and shellbar #4369
Conversation
packages/fiori/src/ShellBar.hbs
Outdated
@@ -107,6 +109,7 @@ | |||
icon="sap-icon://search" | |||
data-ui5-text="Search" | |||
data-ui5-notifications-count="{{notificationsCount}}" | |||
data-ui5-stable="searchButton" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data-ui5-stable="searchButton" | |
data-ui5-stable="search" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one at least should say button, because I think the search itself has "search" and this is just the magnifying glass icon.
packages/main/src/TabContainer.hbs
Outdated
@@ -11,6 +11,7 @@ | |||
<div class="{{classes.headerBackArrow}}"> | |||
<ui5-button @click="{{_onHeaderBackArrowClick}}" | |||
icon="slim-arrow-left" | |||
data-ui5-stable="tabsLeft" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): Should we be using kebab case for the "tabsLeft"
, "tabsRight"
, and "tabsOverflow"
values of the newly introduced data-ui5-stable
attributes in this file?
… in a review Co-authored-by: Stoehr <stoehr@gmail.com>
packages/fiori/src/ShellBar.hbs
Outdated
@@ -107,6 +109,7 @@ | |||
icon="sap-icon://search" | |||
data-ui5-text="Search" | |||
data-ui5-notifications-count="{{notificationsCount}}" | |||
data-ui5-stable="search-button" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): If we want to distinguish the button that executes the search, with the field that is used for the search input, then how about having this data-ui5-stable
attribute be "execute-search"
instead of "search-button"
or "search"
, so that it is similar to the "cancel-search"
button, in that the "button" role isn't actually used in the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The internal way of how StableDomRef concept works is now changed.
In order to make this work, you should create getters for all of the data-ui5-stable
attributes that you have added.
You can take a look at how is the logo getter implemented:
In ShellBar.js:
/**
* Returns the <code>logo</code> DOM ref.
* @type { HTMLElement }
* @public
* @readonly
* @since 1.0.0-rc.16
*/
get logoDomRef() {
return this.shadowRoot.querySelector(`*[data-ui5-stable="logo"]`);
}
Shellbar.hbs:
<span
...
data-ui5-stable="logo"
>
<slot name="logo"></slot>
</span>
As we discussed in a meeting, this good feedback should be done in a separate PR, since this PR needs to have a more basic solution that can be patched in a version that pre-dates 1.0.x. |
Thank you for your contribution! 👏
To get it merged faster, kindly review the checklist below:
Pull Request Checklist