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

AG-11450 - More new DOM structure fixes. #1569

Merged
merged 9 commits into from May 13, 2024

Conversation

alantreadway
Copy link
Member

@alantreadway alantreadway commented May 10, 2024

https://ag-grid.atlassian.net/browse/AG-11450

More refinements of the new DOM:

  • Tooltips now respect window bounds correctly, and are no longer clipped due to removal of redundant overflow: hidden in several spots.
  • Creating a chart into a DOM with ZERO CSS styles now renders a chart correctly.
  • Setting chart width and/or height now correctly auto-centers the chart in the container DIV.
    • Additionally, tooltips, focus box and other overlaid elements now appear in the correct positions relative to the canvas.
  • Reduction of number of inline styles in favour of CSS-based styles when feasible.

Example-runner cleanup:

  • Stop polluting the container element:
    • Moved data-ag-charts onto .ag-charts-wrapper which we manage.

Use-cases tested - configs:

  • Zero CSS / auto-sized chart ✅
  • Zero CSS / fixed-sized chart ✅
  • Container fixed/% / auto-sized chart ✅
  • Container fixed/% / fixed-size chart ✅

Features tested:

  • Tooltips ✅
  • Keyboard Nav ✅
  • Crosshairs ✅
  • Context Menu ✅
  • Zoom buttons ✅

@alantreadway alantreadway requested a review from a team as a code owner May 10, 2024 13:51
@@ -172,12 +172,10 @@ export abstract class Chart extends Observable implements AgChartInstance {
newValue(value: HTMLElement) {
if (this.destroyed) return;

value.setAttribute('data-ag-charts', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

We use this for the dark mode logic (plugins/ag-charts-generate-example-files/src/executors/generate/generator/utils/getDarkModeSnippet.ts)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I've moved this onto the part of the DOM we actually manage and updated the dark-mode logic.

right: 0;
.ag-charts-canvas-overlay {
position: relative;
display: contents;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's display: contents meant to be doing here?

Copy link
Member Author

Choose a reason for hiding this comment

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

In one iteration it seemed that the <div> was adding some extra 3px to the canvas size (which caused an infinitely growing canvas feedback loop), this seemed to remove any extra contribution of this element from the impact on the parents size.

Copy link
Contributor

Choose a reason for hiding this comment

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

The infinite feedback loop happens when you miss display: block from the canvas - it defaults to inline-block, which creates extra padding

@alantreadway alantreadway merged commit 85eb31e into latest May 13, 2024
11 checks passed
@alantreadway alantreadway deleted the ag-11450/tooltip-bounds-fix branch May 13, 2024 10:11
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.

None yet

2 participants