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.

Sign up for GitHub

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

Jump to bottom

refactor(menu-button): added support for icons and bug fixes #135

Merged
merged 5 commits into from
Jan 29, 2024
Merged

refactor(menu-button): added support for icons and bug fixes #135

merged 5 commits into from
Jan 29, 2024

Conversation

saiponnada
Copy link
Contributor

@saiponnada saiponnada commented Jan 18, 2024 •
edited
Loading

Fixes #91, #134

  • Fixes setting aria-haspopup attribute to true programmatically.
  • Added configuration option buttonValueType that takes one of these values [text, icon, both]
  • For buttonValueType either icon or both, iconSelector option is used to select icon element.
  • No changes were made to makeup-menu.
  • Updated examples on both makeup-menu and makeup-menu-button. 

Screenshots

makeup-menu

image

makeup-menu-button

image image

@saiponnada saiponnada self-assigned this Jan 18, 2024
ArtBlue
ArtBlue requested changes Jan 19, 2024
View reviewed changes
docs/makeup-menu-button/index.html Show resolved Hide resolved
docs/makeup-menu-button/index.js Outdated Show resolved Hide resolved
docs/makeup-menu/index.js Outdated Show resolved Hide resolved
packages/makeup-menu-button/dist/cjs/index.js Outdated Show resolved Hide resolved
packages/makeup-menu-button/dist/mjs/index.js Outdated Show resolved Hide resolved
packages/makeup-menu-button/src/index.js Outdated Show resolved Hide resolved
ArtBlue
ArtBlue requested changes Jan 19, 2024
View reviewed changes
packages/makeup-menu-button/src/index.js Outdated Show resolved Hide resolved
packages/makeup-menu/src/index.js Outdated Show resolved Hide resolved
LuLaValva
LuLaValva reviewed Jan 24, 2024
View reviewed changes
Copy link
Contributor

@LuLaValva LuLaValva left a comment

Choose a reason for hiding this comment

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

Looks awesome! Just one quick comment.

packages/makeup-menu-button/src/index.js Outdated Show resolved Hide resolved
@saiponnada saiponnada changed the title feat(menu-button): added support for icons refactor(menu-button): added support for icons and bug fixes Jan 26, 2024
LuLaValva
LuLaValva approved these changes Jan 26, 2024
View reviewed changes
Copy link
Contributor

@LuLaValva LuLaValva left a comment

Choose a reason for hiding this comment

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

Nice refactor. I'm happy approving as-is, but it does feel a little weird that the icon case is so specific. I can't think of a more generic solution right now, so I'll approve, but it would be nice if this was more general.

@saiponnada saiponnada merged commit 57fd50d into master Jan 29, 2024
1 check passed
@saiponnada saiponnada deleted the menu-button-changes branch March 14, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArtBlue ArtBlue ArtBlue requested changes

@LuLaValva LuLaValva LuLaValva approved these changes

@ianmcburnie ianmcburnie Awaiting requested review from ianmcburnie

@agliga agliga Awaiting requested review from agliga

Assignees

@saiponnada saiponnada

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

makeup-menu-button: add aria-haspopup if missing
3 participants
@saiponnada @ArtBlue @LuLaValva