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

Proof of concept to add a visual coordinates grid #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

didoo
Copy link

@didoo didoo commented Apr 30, 2020

This is (super-raw) proof of concept that I have prepared, to add a grid of coordinates below the SVG "elements", to help visualize (and understand) the relation between the numbers in the path and the coordinates system of the SVG, and discuss if proceed from here.

This is what it looks like (again, a general idea):
screenshot_4472

I have opened this PR to see if makes sense also for you, and in that case discuss what is the best way to implement it in the rendered SVG. eg. the axis should cover also the negative coordinates? should we add a coordinate indicator on hover? other things that can help visualize the relation between M 140,20 and the fact that this refers to the coordinates (x=140,y=20)? should I introduce an helper to calculate a "scale-factor" that automatically adjusts the entire grid, based on the min/max X/Y values?

(of course, in that case, I'll write proper code, and use the style() function to style the lines) don't worry. this was a quick hack done before dinner)

@mathieudutour
Copy link
Owner

That's amazing 👏

the axis should cover also the negative coordinates?

Depending on path, it could be negative yes

should we add a coordinate indicator on hover?

When you hover on a coordinate on the explanation, it shows an indicator already. Are you thinking of thinking else?

should I introduce an helper to calculate a "scale-factor" that automatically adjusts the entire grid, based on the min/max X/Y values?

That should already be done if you use the bounds?

I'm wondering if showing this grid could be a setting: a 🛠️ in the bottom right corner which would show a set of checkboxes to control what we show. I can think of a couple of ones:

  • showing this grid
  • showing the control points
  • a scale factor perhaps? (eg. use the same scale as the path or scale as much as we can)

Comment on lines +626 to +629
const gridElements = {
lines: [],
labels: [],
} as any; // TODO TS to fix
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const gridElements = {
lines: [],
labels: [],
} as any; // TODO TS to fix
const gridElements = {
lines: [] as React.ReactNode[],
labels: [] as React.ReactNode[],
};

['x', 'y'].forEach((axis) => {
// not very elegant, but it works
// TODO alternative, use some "magic" code that uses "object.keys(gridProps) and works out the "other" key ?
const otherAxis = axis === 'x' ? 'y' : 'x';
Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine with this, clear code beats magic code, especially when there are only 2 cases haha

);

if (isMultipleOfPow10Scale) {
const labelCoordinates = {} as any; // TODO TS to fix
Copy link
Owner

Choose a reason for hiding this comment

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

I think you can have

const labelCoordinates = {x: 0, y: 0}

so that TS doesn't complain.

Or NaN if you want to show that it's not ready yet

@darkworks
Copy link

why its not merged yet ?

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

3 participants