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

fix(core): fix issue where legend component ids are not unique #1688

Closed

Conversation

joker23
Copy link

@joker23 joker23 commented Nov 16, 2023

#1619

Updates

this commit will initialize a chartId in the domutils class when an instance of the class is created... this ensures that any ids generated by the domutils can be unique based on that id

Demo screenshot or recording

chart legend id will now have a unique id component
image

@joker23 joker23 requested review from theiliad and a team as code owners November 16, 2023 23:17
Copy link

netlify bot commented Nov 16, 2023

Deploy Preview for carbon-charts-react ready!

Name Link
🔨 Latest commit 0a1abd6
🔍 Latest deploy log https://app.netlify.com/sites/carbon-charts-react/deploys/6570fb482fdbd2000897b55c
😎 Deploy Preview https://deploy-preview-1688--carbon-charts-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 16, 2023

Deploy Preview for carbon-charts-core ready!

Name Link
🔨 Latest commit 0a1abd6
🔍 Latest deploy log https://app.netlify.com/sites/carbon-charts-core/deploys/6570fb482609b60008c89f41
😎 Deploy Preview https://deploy-preview-1688--carbon-charts-core.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 16, 2023

Deploy Preview for carbon-charts-angular ready!

Name Link
🔨 Latest commit 0a1abd6
🔍 Latest deploy log https://app.netlify.com/sites/carbon-charts-angular/deploys/6570fb4846169200083d09e6
😎 Deploy Preview https://deploy-preview-1688--carbon-charts-angular.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@carbon-bot
Copy link
Collaborator

carbon-bot commented Nov 16, 2023

DCO Assistant Lite bot All contributors have signed the DCO.

this commit will initialize a chartId in the domutils class when an instance of the class is
created... this ensures that any ids generated by the domutils can be unique based on that id

carbon-design-system#1619
@joker23 joker23 force-pushed the fix-id-collision-in-legend branch from 8492590 to 9407f9a Compare November 17, 2023 16:08
@joker23
Copy link
Author

joker23 commented Nov 17, 2023

I have read the DCO document and I hereby sign the DCO.

@theiliad
Copy link
Member

Copy link
Member

@theiliad theiliad left a comment

Choose a reason for hiding this comment

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

comment above

@joker23
Copy link
Author

joker23 commented Dec 1, 2023

Hey @theiliad - I did a quick test and it looks like the dom-util instance used by the chart and the legend are different? What I did was lazy load the chart id per instance by modifying the getChartID function to:

	getChartID() {
		if (!this.chartID) {
			this.initializeID()
		}

		return this.chartID
	}

Theoretically if the dom util service is shared between the chart and the legend then they should be the same value, but I got:
image

Thoughts?

For what it's worth, the chart ids today between the wrapper and legend doesn't match up so it's probably okay?

@theiliad
Copy link
Member

theiliad commented Feb 2, 2024

I believe this was fixed by @RiyaJethwa. IDs seem to be correct on storybook, pls lmk if this PR still applies

@theiliad theiliad closed this Feb 2, 2024
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.

3 participants