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

fix(localemodal): disable region button when no countries present #7840

Merged
merged 3 commits into from
Dec 10, 2021
Merged

fix(localemodal): disable region button when no countries present #7840

merged 3 commits into from
Dec 10, 2021

Conversation

kennylam
Copy link
Member

@kennylam kennylam commented Dec 8, 2021

Related Ticket(s)

#7831

Description

Fixes a bug in LocaleModal where the region button is still clickable even when no countries of that region are available.

Note: Europe has been disabled for testing purposes.

Dec-08-2021 16-38-07

Changelog

Changed

  • update event handler for region buttons with no countries/languages available for localization
  • sort country list in preview.html by region

@kennylam kennylam added the package: react Work necessary for the Carbon for IBM.com react components package label Dec 8, 2021
@kennylam kennylam requested a review from a team as a code owner December 8, 2021 21:42
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 8, 2021 •
edited
Loading

Deploy preview created for package React:
https://ibmdotcom-react.s3.us-south.cloud-object-storage.appdomain.cloud/deploy-previews/7840/index.html

Built with commit: aac56626274ab669c86058b7204d2aedadfef1dc

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 8, 2021 •
edited
Loading

Deploy preview created for package Web Components HTML Test Application:
https://ibmdotcom-web-components-html-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7840/index.html

Built with commit: aac56626274ab669c86058b7204d2aedadfef1dc

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 8, 2021 •
edited
Loading

Deploy preview created for package Web Components:
https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7840/index.html

Built with commit: aac56626274ab669c86058b7204d2aedadfef1dc

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 8, 2021 •
edited
Loading

Deploy preview created for package Web Components (React wrapper):
https://ibmdotcom-web-components-react.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7840/index.html

Built with commit: aac56626274ab669c86058b7204d2aedadfef1dc

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 8, 2021 •
edited
Loading

Deploy preview created for package NextJS Test Application:
https://ibmdotcom-nextjs-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7840/index.html

Built with commit: aac56626274ab669c86058b7204d2aedadfef1dc

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 8, 2021 •
edited
Loading

Deploy preview created for package "Web Components (Codesandbox Examples)":
https://webcomponents-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7840/index.html

Built with commit: aac56626274ab669c86058b7204d2aedadfef1dc

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 9, 2021 •
edited
Loading

Deploy preview created for package "React (Codesandbox Examples)":
https://react-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7840/index.html

Built with commit: aac56626274ab669c86058b7204d2aedadfef1dc

emyarod
emyarod approved these changes Dec 9, 2021
View reviewed changes
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me! the empty region is disabled properly now

@jeffchew jeffchew added Ready to merge Label for the pull requests that are ready to merge and removed Ready to merge Label for the pull requests that are ready to merge labels Dec 9, 2021
@jeffchew
Copy link
Member

jeffchew commented Dec 9, 2021

@kennylam hey for web components, the tile doesn't look disabled:

image

How it looks in React:

image

@emyarod emyarod mentioned this pull request Dec 9, 2021
Closed
emyarod
emyarod reviewed Dec 9, 2021
View reviewed changes
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

after rechecking on narrower viewports, it looks like there's a little weird hover effect on the disabled tile in web components

Screen.Recording.2021-12-09.at.11.05.59.AM.mov

@emyarod emyarod self-requested a review December 9, 2021 17:07
@kennylam
Copy link
Member Author

kennylam commented Dec 9, 2021

@jeffchew @emyarod Both of those are existing issues that should probably be addressed separately:

  • the disabled styles for web components card-link do not match the React (and Carbon) version
  • the disabled icon shifting position on hover can be seen here

I'll open tickets for both of these issues.

emyarod
emyarod approved these changes Dec 9, 2021
View reviewed changes
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

sounds good! then I think we can merge after restoring the removed locales for testing

@kennylam
Copy link
Member Author

kennylam commented Dec 9, 2021

sounds good! then I think we can merge after restoring the removed locales for testing

I left Europe disabled to match the web components version made in this PR. Should we enable them again?

@jeffchew jeffchew added the Ready to merge Label for the pull requests that are ready to merge label Dec 9, 2021
@jeffchew
Copy link
Member

jeffchew commented Dec 9, 2021

sounds good! then I think we can merge after restoring the removed locales for testing

I left Europe disabled to match the web components version made in this PR. Should we enable them again?

No let's keep them both to match where Europe is disabled.

@jeffchew jeffchew merged commit 927c32e into carbon-design-system:master Dec 10, 2021
@kennylam kennylam deleted the fix/locale-modal-region-button-disabled branch January 6, 2022 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emyarod emyarod emyarod approved these changes

@IgnacioBecerra IgnacioBecerra Awaiting requested review from IgnacioBecerra IgnacioBecerra was automatically assigned from carbon-design-system/carbon-for-ibm-com-members

@JBrandwine JBrandwine Awaiting requested review from JBrandwine JBrandwine was automatically assigned from carbon-design-system/carbon-for-ibm-com-members

Assignees
No one assigned
Labels
package: react Work necessary for the Carbon for IBM.com react components package Ready to merge Label for the pull requests that are ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kennylam @ibmdotcom-bot @jeffchew @emyarod