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

Add menu icon class #331

Merged
merged 3 commits into from
Oct 29, 2019
Merged

Add menu icon class #331

merged 3 commits into from
Oct 29, 2019

Conversation

Martskin
Copy link
Contributor

@Martskin Martskin commented Oct 14, 2019

Adds spectrum-Menu-itemIcon classname.

Description

Adds menu specific icon classname for issue #202 and changes all menu instance with icons.

Screen Shot 2019-10-14 at 1 18 05 PM

Copy link
Member

@lazd lazd left a comment

Choose a reason for hiding this comment

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

Thanks! There's one more instance of .spectrum-Icon in the CSS, and we need backwards compatibility.

@@ -99,7 +99,7 @@ governing permissions and limitations under the License.
}
}

.spectrum-Icon {
.spectrum-Menu-itemIcon {
/* Don't get smaller, you're an icon! */
flex-shrink: 0;
align-self: flex-start;
Copy link
Member

Choose a reason for hiding this comment

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

There's another instance below that needs this change. I would say we should also preserve support for the existing markup so we don't have to make a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh! Good catch. Fixed!

@lazd
Copy link
Member

lazd commented Oct 16, 2019

This looks good. I'd say it's a feat since we're adding a new class in a backwards compatible way.

@Martskin
Copy link
Contributor Author

This looks good. I'd say it's a feat since we're adding a new class in a backwards compatible way.

Thanks @lazd ! Do I need to add a new commit with feat to this PR?

@lazd lazd self-assigned this Oct 17, 2019
@lazd lazd merged commit 169940a into adobe:master Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants