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

UI: Fix the missing pixels issue with distribution-bar #4507

Merged
merged 3 commits into from
Jul 13, 2018

Conversation

DingoEatingFuzz
Copy link
Contributor

These two pixels have been a thorn in my side for a long time.

This fixes two bugs that amount to the same missing pixel issue:

  1. Subtraction based on index was using the wrong index due to slices being filtered out when they have a value of 0.
  2. Subtracting 1 instead of 2 when a slice is both the first and last slice in the set.

Here are some very subtle before and after screenshots:

Bug 1: A missing pixel on the right side due to wrong indices

before
screen shot 2018-07-12 at 4 43 46 pm

after
image

Bug 2: A missing pixel on either side due to subtracting only 1 instead of 2.

before
screen shot 2018-07-12 at 4 40 22 pm

after
screen shot 2018-07-12 at 4 40 37 pm

@@ -76,6 +76,9 @@ export default Component.extend(WindowResizable, {
const { chart, _data, isNarrow } = this.getProperties('chart', '_data', 'isNarrow');
const width = this.$('svg').width();
const filteredData = _data.filter(d => d.value > 0);
filteredData.forEach((d, index) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I read a thing about preference of for ... of over forEach loops recently and have been trying to use them more. I certainly like they way they read, though that's not necessarily true of the transpiled code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always forget that JS has better for loops now.

// Remove a pixel from either side of the slice
let modifier = 2;
if (d.index === 0) modifier--; // But not the left side
if (d.index === sliceCount - 1) modifier--; // But not the right side
Copy link
Contributor

Choose a reason for hiding this comment

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

These three lines are clever (in a good way).

Copy link
Contributor

@meirish meirish left a comment

Choose a reason for hiding this comment

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

Nice improvement - I never knew they weren't square corners!

@DingoEatingFuzz DingoEatingFuzz force-pushed the b-ui-dist-bar-corners branch from ddd40cb to d5af5e6 Compare July 13, 2018 17:26
@DingoEatingFuzz DingoEatingFuzz merged commit 62cdc28 into master Jul 13, 2018
@DingoEatingFuzz DingoEatingFuzz deleted the b-ui-dist-bar-corners branch July 13, 2018 18:04
@github-actions
Copy link

github-actions bot commented Mar 1, 2023

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants