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

fix: adding stable selectors for tab container and shellbar #4369

Merged
merged 3 commits into from
Nov 23, 2021

Conversation

codefactor
Copy link
Contributor

@codefactor codefactor commented Nov 20, 2021

Thank you for your contribution! 👏

To get it merged faster, kindly review the checklist below:

Pull Request Checklist

packages/fiori/src/ShellBar.hbs Outdated Show resolved Hide resolved
@@ -107,6 +109,7 @@
icon="sap-icon://search"
data-ui5-text="Search"
data-ui5-notifications-count="{{notificationsCount}}"
data-ui5-stable="searchButton"
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking):

Suggested change
data-ui5-stable="searchButton"
data-ui5-stable="search"

Copy link
Contributor Author

@codefactor codefactor Nov 20, 2021

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/fiori/src/ShellBar.hbs Outdated Show resolved Hide resolved
packages/fiori/src/ShellBar.hbs Outdated Show resolved Hide resolved
@@ -11,6 +11,7 @@
<div class="{{classes.headerBackArrow}}">
<ui5-button @click="{{_onHeaderBackArrowClick}}"
icon="slim-arrow-left"
data-ui5-stable="tabsLeft"
Copy link
Contributor

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>
@@ -107,6 +109,7 @@
icon="sap-icon://search"
data-ui5-text="Search"
data-ui5-notifications-count="{{notificationsCount}}"
data-ui5-stable="search-button"
Copy link
Contributor

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?

Copy link
Contributor

@fifoosid fifoosid left a 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>

@stoehr
Copy link
Contributor

stoehr commented Nov 23, 2021

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.

@fifoosid fifoosid dismissed their stale review November 23, 2021 08:21

This change is not downported

@ilhan007 ilhan007 merged commit 9abdaba into SAP:master Nov 23, 2021
ndeshev pushed a commit to ndeshev/ui5-webcomponents that referenced this pull request Dec 13, 2021
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.

6 participants