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

Refactor core.layouts #6304

Merged
merged 1 commit into from
Jun 19, 2019
Merged

Refactor core.layouts #6304

merged 1 commit into from
Jun 19, 2019

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented May 28, 2019

Inspired by #6300, took another look at layouts.

Small changes hidden in a huge refactor :)

  • include margins in padding, not just the difference
  • consider padding in addition to minimum box sizes after step 4
    • this reduces chart area from sides where there are no boxes and some axis requires padding there
  • use minRotation instead of maxRotation in _getLabelSize for vertical time scale.
    • maxRotation defaults to 50, but vertical axis ticks are only rotated to minRotation.
  • changed label format for a test block because ff / chrome kept ending up with different ticks.

This might be a bit faster than master, due to using for instead of helpers.each and caching stuff to a "layout box" container.

CodePen

Closes: #6300

src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member Author

kurkle commented May 28, 2019

Thanks for the reviews @benmccann!

I did another round around this, largely based on your comments. I also combined some functions, although CC now complains about them. But I don't think splitting to more functions makes it really simpler.

src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/scales/scale.time.js Show resolved Hide resolved
test/specs/scale.linear.tests.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
var chartAreaWidth = chartArea.w / 2; // min 50%

// Steps 2-3
var params = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that chartArea is mutated while the rest are read-only constants. What if we take chartArea out of this and pass it separately and then call Object.freeze on params. I think that would help a lot in keeping track of which variables change between function calls.

I also wonder if we need to keep track of chartArea. It seems like there's a lot of complexity in keeping outerDims and chartArea in sync with each other and the boxes at all times. But isn't chartArea.w just outerWidth - outerDims.left - outerDims.right? I think it'd be massively simpler if we just had to worry about keeping outerDims updated. I think we do a pretty good job keeping outerDims.left updated, but there might be a couple places where we skip updating outerDims.right and so we'd have to check that we always keep it updated in order to make that work

Copy link
Contributor

Choose a reason for hiding this comment

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

@kurkle I think this is the only comment I had that hasn't been addressed yet. I resolved the rest.

I'd like if we could at least add a comment saying that w and h are the only mutable properties and the rest are read-only or possibly we could go further as I mentioned above

Besides that, this PR looks good to me. Thank you so much for all the work on this. I'm very excited about it!

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll take another look at this later, because I feel you are right and this part can be simplified.

src/core/core.layouts.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member Author

kurkle commented May 29, 2019

@benmccann comments made me realize this can be done with a round less of updates.
This can probably be re-arranged to be a lot simpler still.

src/core/core.layouts.js Outdated Show resolved Hide resolved
@kurkle

This comment has been minimized.

src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member Author

kurkle commented Jun 3, 2019

Noticed that getPixelForTick gets called with -1 in some of tests (when there is no data).
This results in weird coordinates (and thus padding). Updated all implementations to return null if index is out of bounds.

Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

looks pretty good to me. I'm quite excited about this! just a few minor comments (in addition to a few from the last round that haven't been responded to)

src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
@nagix
Copy link
Contributor

nagix commented Jun 4, 2019

When the x axis is a category scale, and the legend is on the left, the second tick label sometimes disappears.

{
    type: 'line',
    data: {
        labels: ['ABCDEFG', 'ABCDEFG'],
        datasets: [{
            data: [0, 1]
        }]
    },
    options: {
        responsive: false,
        legend: {
            display: true,
            position: 'left'
        }
    }
}

Canvas width: 350px
Screen Shot 2019-06-04 at 1 20 24 PM
Canvas width: 400px
Screen Shot 2019-06-04 at 1 20 47 PM
Canvas width: 450px
Screen Shot 2019-06-04 at 1 21 02 PM

@kurkle
Copy link
Member Author

kurkle commented Jun 4, 2019

Thanks @nagix for testing!

Fixed, _autoSkip was still considering paddings, and its none of scales business really.

I think quite a lot of tests should still be written to complete these changes.

@nagix
Copy link
Contributor

nagix commented Jun 4, 2019

The left padding of the legend is inconsistent.

{
    type: 'line',
    data: {
        labels: ['ABCDEFG', 'ABCDEFG', 'ABCDEFG', 'ABCDEFG'],
        datasets: [{
            data: [0, 1, 2, 3]
        }]
    },
    options: {
        responsive: false,
        legend: {
            display: true,
            position: 'left'
        }
    }
}

Canvas width: 500px, height: 150px
Screen Shot 2019-06-04 at 3 01 01 PM
Canvas width: 500px, height: 100px
Screen Shot 2019-06-04 at 3 01 09 PM
Canvas width: 300px, height: 100px
Screen Shot 2019-06-04 at 3 01 21 PM
Canvas width: 300px, height: 80px
Screen Shot 2019-06-04 at 3 31 12 PM

@nagix
Copy link
Contributor

nagix commented Jun 4, 2019

Maybe the first and second update calls for y-axis are generating different tick labels...

@kurkle
Copy link
Member Author

kurkle commented Jun 4, 2019

Update (fixes wrong padding of legend): removed the margins logic from scales. Its already handled by layout.
Another thing removed is fullWidth, but I'm not sure I get the use case it was made for.

If I these changes broke something, then we are missing tests 😄

@nagix
Copy link
Contributor

nagix commented Jun 6, 2019

The legend is not center-aligned.

{
    type: 'line',
    data: {
        labels: ['ABCDEFG', 'ABCDEFG'],
        datasets: [{
            data: [0, 1]
        }]
    },
    options: {
        responsive: false
    }
}

Canvas width: 200px, height: 100px
Screen Shot 2019-06-06 at 10 28 22 AM
Canvas width: 200px, height: 100px
Screen Shot 2019-06-06 at 10 28 33 AM

@kurkle
Copy link
Member Author

kurkle commented Jun 6, 2019

Thanks again, @nagix! Broke that in previous commit. Fixed now.

@benmccann
Copy link
Contributor

There's one currently failing test

@nagix
Copy link
Contributor

nagix commented Jun 6, 2019

@kurkle Thank you for the fix. One concern about getting rid of margins is that it has been a public property for long time and it might break custom scales that are using it. Is it possible to keep it for backward compatibility?

I also feel that the name scale.padding{Top|Right|Bottom|Left} is confusing because it is actually a margin outside the scale rectangle, not a padding inside. However, they are also public properties, so we shouldn't touch them.

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants