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

revert(button): revert changes to button for v11 #10510

Merged

Conversation

joshblack
Copy link
Contributor

Closes #10505
Closes #10294

This PR reverts the changes we made to button for icon support. This should address most of the issues with components using Button as an icon button.

Changelog

New

Changed

  • Revert changes to button to mirror v10 behavior

Removed

Testing / Reviewing

  • Verify issues identified in referenced issues have been resolved
  • Verify IconButton still works as intended

@joshblack joshblack requested a review from a team as a code owner January 24, 2022 21:06
@netlify
Copy link

netlify bot commented Jan 24, 2022

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: e79b81e

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/61f18683182200000850e94e

😎 Browse the preview: https://deploy-preview-10510--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Jan 24, 2022

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: e79b81e

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/61f18683510f0e00070830ff

😎 Browse the preview: https://deploy-preview-10510--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Jan 24, 2022

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: e79b81e

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61f18683cdf15a0008530851

😎 Browse the preview: https://deploy-preview-10510--carbon-components-react.netlify.app

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

This also fixes the Pagination forward/back buttons issue @tw15egan discovered today

Copy link
Contributor

@jnm2377 jnm2377 left a comment

Choose a reason for hiding this comment

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

I can see the buttons now, but they still have some weird alignment issues with the icons.
Screen Shot 2022-01-25 at 11 04 48 AM

@joshblack
Copy link
Contributor Author

@jnm2377 good spot, thanks for catching that. It seems like our reset helper has changed a bit and now will set display: block 🤔

Let me go and update that real quick!

@joshblack joshblack requested a review from jnm2377 January 25, 2022 21:37
@tw15egan tw15egan mentioned this pull request Jan 26, 2022
29 tasks
Copy link
Contributor

@jnm2377 jnm2377 left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

@kodiakhq kodiakhq bot merged commit 885c987 into carbon-design-system:main Jan 26, 2022
@tw15egan
Copy link
Collaborator

@joshblack noticing that the tooltips do not appear when hovering over Icon buttons (noticed in Pagination). Is this new behavior intentional?

@joshblack
Copy link
Contributor Author

@tw15egan great catch, this is currently coming up because we are not including the tooltip styles in v11 and are trying to supplement them with popover/icon button usage.

I think this is something that we can ship as a known issue and use the next couple of sprints to either:

  • Restore the tooltip styles for button tooltips
  • Update our internal usage of Button to use the new IconButton
  • Some other option??? lol

Let me know what you think!

@tw15egan
Copy link
Collaborator

@joshblack gotcha! I see that they are working as intended in the v10 source just not in v11. I think option 2 makes the most sense going forward in the v11 source.

@abbeyhrt abbeyhrt mentioned this pull request Jan 27, 2022
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants