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

feat: unify DOM properties for highlight element across all plots #539

Open
3 of 5 tasks
krishnanand5 opened this issue Aug 29, 2024 · 1 comment
Open
3 of 5 tasks
Labels
migration a scenario to consider during TS migration

Comments

@krishnanand5
Copy link
Collaborator

Migration Refactor/Reorganization

Description

This issue is to be treated as a placeholder for one of the changes that need to be accounted for during the JS-TS migration. The DOM properties of highlight element across all plots need to be unified and streamlined to allow for easy manipulation whenever required.

Motivation

Currently, we use different DOM properties for highlight element in each plot - some highlight elements are addressed using a class and some using an ID. Even the IDs and classes are not similar and this forces multiple lines of code to address events in different plots that require DOM manipulation.

Similarly, stroke and fill are two attributes that have been used interchangeably across all highlight elements. These attributes are paramount for styling the highlight element and tearing them down or updating them sometimes requires the use of both attributes when one might have been sufficient.

Proposed Solution

Identify one common class or ID for the highlight element across all plots. If CSS is a blocker for the same, identify probable solutions where the DOM manipulation procedures can be clubbed together based on attribute similarities.

Additional Context

The current implementation of DOM manipulation can be trimmed down to produce a more modular approach.

Checklist

  • I have searched for similar feature requests and confirmed that this is a new request.
  • I have provided a clear and concise description of the feature.
  • I have explained the motivation behind this feature request.
  • I have outlined a proposed solution or ideas for implementing this feature.
  • I have provided any additional context or screenshots if applicable.
@krishnanand5 krishnanand5 added the migration a scenario to consider during TS migration label Aug 29, 2024
@krishnanand5 krishnanand5 changed the title feat: DOM properties for highlight element across all plots feat: unify DOM properties for highlight element across all plots Aug 29, 2024
@ellvix
Copy link
Collaborator

ellvix commented Aug 29, 2024

I expect this is not possible with the current SVGs from ggplot etc. The DOM structure for the SVGs is just fundamentally different between different chart types. The current selectors used for each have already been refined a few times and are as general as we could get after some effort.

If we want to standardize the selectors / DOM properties, I see 2 options:

  • Modify ggplot and other tools that create the SVG files so that they output a consistent structure. This sounds annoying and likely not possible.
  • On Init(), create code to manually restructure the SVG DOM to be consistent so we can work with it. This sounds doable, but I don't see any difference between creating (and maintaining) a DOM converter function, and just working with the DOM as is. Well, one small difference, and that's that the user wouldn't have to choose a selector, but that's still similar to now where they can use the default selector we've already gotten working. If we encounter a new SVG structure, everything will break again and then we're back to where we are now.

@krishnanand5 thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migration a scenario to consider during TS migration
Projects
None yet
Development

No branches or pull requests

2 participants