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

Every call to the ClarityIcons.addIcons is triggering CDS_UPDATE_STATE, even though that icon is already available in registry #332

Open
akshay-shekokar opened this issue Oct 7, 2024 · 0 comments

Comments

@akshay-shekokar
Copy link

akshay-shekokar commented Oct 7, 2024

Please review our Contribution guide for more details about how to contribute effectively.

Every call to the ClarityIcons.addIcons is triggering CDS_UPDATE_STATE, even though that icon is already available in registry

RCA:

In this file:
https://github.com/vmware-clarity/core/blob/main/projects/core/src/icon/icon.service.ts#L31C3-L36C4

for addIcons method:
static addIcons(...shapes: IconShapeTuple[]) {
GlobalStateService.state.iconRegistry = {
...GlobalStateService.state.iconRegistry,
...Object.fromEntries(shapes.filter(([name]) => !ClarityIcons.registry[name])),
};
}

addIcons method is creating new object and assigning it to GlobalStateService.state.iconRegistry, which seems like triggering event CDS_UPDATE_STATE.

Instead, we should check in addIcons that whether this icon is registered or not, if the icon is already registered then we should not create new object.

Please check if it works here
static addIcons(...shapes: IconShapeTuple[]) {
const shapesToRegister = shapes.filter(([name]) => !ClarityIcons.registry[name]),
if (shapesToRegister.length) {
GlobalStateService.state.iconRegistry = {
...GlobalStateService.state.iconRegistry,
...Object.fromEntries(shapesToRegister),
};
}
}

Summary

Describe the change or feature in detail. Try to answer the following questions.

  • What is the change?
  • Why should it go in Clarity?
  • Does this change impact existing behaviors? If so how?
  • If this change introduces a new behavior, is this behavior accessible?

Use case

Provide some specific details about how this change would improve your application and use case.

Examples

If possible, show examples of the change. You may create one yourself, or link to external sites that have the idea. It can also be very useful to prototype the idea in isolation outside of Clarity with a Stackblitz example.

API

Describe the intended API for the feature you want to add. This would include:

  • CSS classes and DOM structure for pure static UI contribution, and inputs/outputs, components, directives, services, or anything that is exported publicly for Angular contributions.
  • Examples of code snippets using this new feature.
  • Note very clearly if anything might be a breaking change.

In the case of bug fixes or internal changes, there will most likely be no API changes.

Implementation plan

Describe how you plan to implement the feature, answering questions among the following or anything else you deem relevant.

  • What parts of the code are affected?
  • Will you introduce new services, components or directives?
  • Can you describe the basic flow of information between classes?
  • Does this introduce asynchronous behaviors?
  • Will you need to refactor existing Clarity code?
  • Will reasonable performance require optimizations?
  • Will it need to access native elements (and be incompatible with server-side rendering)?
  • ...

Conclusion

Describe how long you expect it to take to implement, what help you might need, and any other details that might be helpful. Don't worry, this is obviously non-contractual. 😛

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

No branches or pull requests

1 participant