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(parent-visibility): decouple tab from parent visibility #11759

Closed
wants to merge 7 commits into from
Closed

fix(parent-visibility): decouple tab from parent visibility #11759

wants to merge 7 commits into from

Conversation

m4olivei
Copy link
Contributor

@m4olivei m4olivei commented May 1, 2024 •
edited

Related Ticket(s)

Closes #11749

Description

This decouples C4DTab from ParentVisibilityMixin, which avoids the sideeffect of registering the c4d-tab component, when you're otherwise just making use of ParentVisibilityMixin.

Changelog

Changed

  • Avoids referencing C4DTab from ParentVisibilityMixin by duplicating the event name.

@m4olivei m4olivei added bug Something isn't working severity 3 Affects minor functionality, has a workaround dev Needs some dev work package: web components Work necessary for the IBM.com Library web components package adopter: AEM used when component or pattern will be used by this adopter owner: Innovation Team used when the engineering work will be done by Hybrid Cloud with DDS engineers as consultants labels May 1, 2024
@m4olivei m4olivei requested a review from a team as a code owner May 1, 2024 18:48
@m4olivei m4olivei requested review from kennylam and emyarod and removed request for a team May 1, 2024 18:48
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 1, 2024 •
edited

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/11759/index.html

Built with commit: d521be1da400245c78a928bf9aa8d6a7d4b1284b

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 1, 2024 •
edited

Deploy preview created for package Carbon Web Components:
https://carbon-web-components.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/11759/index.html

Built with commit: d521be1da400245c78a928bf9aa8d6a7d4b1284b

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 1, 2024 •
edited

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

Built with commit: d521be1da400245c78a928bf9aa8d6a7d4b1284b

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 1, 2024 •
edited

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/11759/index.html

Built with commit: d521be1da400245c78a928bf9aa8d6a7d4b1284b

@m4olivei m4olivei requested review from jkaeser and andy-blum May 2, 2024 15:13
@andy-blum andy-blum added the test: CDN preview used for generating @carbon/ibmdotcom-web-components CDN for testing label May 2, 2024
@m4olivei m4olivei requested a review from jkaeser May 3, 2024 13:16
jkaeser
jkaeser reviewed May 3, 2024
View reviewed changes
Copy link
Member

@jkaeser jkaeser left a comment

Choose a reason for hiding this comment

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

The code changes here look good to me, but this is failing ci-check. Can you look into that, @m4olivei ?

@m4olivei
Copy link
Contributor Author

m4olivei commented May 3, 2024

The code changes here look good to me, but this is failing ci-check. Can you look into that, @m4olivei ?

Huh, interesting, appraently there is a rule that requires static get returns to only be a string or string literal:

https://github.com/carbon-design-system/carbon-for-ibm-dotcom/blob/main/packages/web-components/tools/babel-plugin-create-react-custom-element-type.js#L186-L190

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 3, 2024 •
edited

Deploy preview created for package IBM.com Web Components Deploy Preview CDN:
https://ibmdotcom-cdn.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/11759/index.html

Built with commit: d521be1da400245c78a928bf9aa8d6a7d4b1284b

jkaeser
jkaeser requested changes May 3, 2024
View reviewed changes
Copy link
Member

@jkaeser jkaeser left a comment

Choose a reason for hiding this comment

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

Okay, now that we have a representative deploy preview, I've found an issue. <dds-cta-block-item-row> uses ParentVisibilityMixin, and it's not rendering properly within tabs (see https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/11759/index.html?path=/story/components-cta-block--within-tabs).

Screenshot 2024-05-03 at 1 20 39 PM

@m4olivei
Copy link
Contributor Author

m4olivei commented May 3, 2024

Okay, now that we have a representative deploy preview, I've found an issue. <dds-cta-block-item-row> uses ParentVisibilityMixin, and it's not rendering properly within tabs (see https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/11759/index.html?path=/story/components-cta-block--within-tabs).

Screenshot 2024-05-03 at 1 20 39 PM

Thanks @jkaeser . Looks like main has the same problem. I'll look into it.

@m4olivei
Copy link
Contributor Author

m4olivei commented May 3, 2024 •
edited

It turns out that the Tabs Extended component changed structure such that the tab component is no longer a parent to the contents of each tab panel. As such, the ParentVisibilityMixinImpl doesn't work in v2 and a new paradigm is needed for doing the same.

In addition, the story for CTA block > Within tabs was not correctly updated in v2 for the new tabs strucutre alluded to above. This is why it's broken on main.

All that is to say, we're now out of scope for the original ask and we kind of need to take this back to the drawing board for v2, which we'll do in a follow up issue.

Closing for now.

@m4olivei m4olivei closed this May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkaeser jkaeser jkaeser requested changes

@kennylam kennylam Awaiting requested review from kennylam kennylam is a code owner automatically assigned from carbon-design-system/carbon-for-ibm-com-members

@emyarod emyarod Awaiting requested review from emyarod emyarod is a code owner automatically assigned from carbon-design-system/carbon-for-ibm-com-members

@andy-blum andy-blum Awaiting requested review from andy-blum

Assignees
No one assigned
Labels
adopter: AEM used when component or pattern will be used by this adopter bug Something isn't working dev Needs some dev work owner: Innovation Team used when the engineering work will be done by Hybrid Cloud with DDS engineers as consultants package: web components Work necessary for the IBM.com Library web components package severity 3 Affects minor functionality, has a workaround test: CDN preview used for generating @carbon/ibmdotcom-web-components CDN for testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[parent-visibility-mixin]: dds-video imports dds-tab component
4 participants
@m4olivei @ibmdotcom-bot @jkaeser @andy-blum