-
Notifications
You must be signed in to change notification settings - Fork 272
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(ui5-button): set icon-only attribute (#2567) #2824
Conversation
…the ui5-button component (#2567)
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.
Thanks for the contribution! Please see my comments about the blank text button. Otherwise, after a successful build, I'm good to +1 this PR.
packages/main/test/pages/Button.html
Outdated
@@ -29,7 +29,8 @@ | |||
</head> | |||
|
|||
<body style="background-color: var(--sapBackgroundColor);"> | |||
<ui5-button icon="home"><!----><!----></ui5-button> | |||
<ui5-button icon="home" id="icon-only-comment"><!----><!----></ui5-button> | |||
<ui5-button icon="text" id="icon-only-blank-text"></ui5-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.
Shouldn't this example rather be > </ui5-button>
(have a single space inside)?
packages/main/src/Button.js
Outdated
@@ -378,7 +378,10 @@ class Button extends UI5Element { | |||
} | |||
|
|||
get isIconOnly() { | |||
return !Array.from(this.childNodes).filter(node => node.nodeType !== Node.COMMENT_NODE).length; | |||
return !Array.from(this.childNodes).filter(node =>{ |
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.
I think ESLINT will require a space after the =>
operator here
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.
Yes, you are right.
changes for ESLINT
Issue: When a ui5-button is created with blank text inside the component, for example
<ui5-button icon="message-information"> </ui5-button>
, the attribute "icon-only" isn't set correctly.Solution: Trim the node value when node is type of "TEXT_NODE" and check the length of the trimmed value.
FIXES: #2567