Skip to content

fix(color-contrast): account for elements that do not fill entire bounding size#3186

Merged
straker merged 2 commits intodevelopfrom
color-contrast-empty-cells
Oct 6, 2021
Merged

fix(color-contrast): account for elements that do not fill entire bounding size#3186
straker merged 2 commits intodevelopfrom
color-contrast-empty-cells

Conversation

@straker
Copy link
Copy Markdown
Contributor

@straker straker commented Oct 5, 2021

Closes issue: #3166

@straker straker requested a review from a team as a code owner October 5, 2021 17:11
const endRow = ((y + rect.height) / gridSize) | 0;
const endCol = ((x + rect.width) / gridSize) | 0;

grid._numCols = Math.max(grid._numCols ?? 0, endCol);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This feels a little off. If _numCols needs to be different, it should be fixed where that value is set. If it doesn't need to be changed, this should probably just be a local variable.

Copy link
Copy Markdown
Contributor Author

@straker straker Oct 6, 2021

Choose a reason for hiding this comment

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

It needs to be set for every grid, so can't be local. Grid items are added dynamically, so there's no way to know how many rows or columns the grid will have before hand, we can only know as each item is added and changes the grid size.

// can't be an element whose midpoint is at column 5. If this happens this
// means there's an error in our grid logic that needs to be fixed
if (row > grid.cells.length || col > grid._numCols) {
throw new Error('Element midpoint exceeds the grid bounds');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't follow why you would want this to throw. Crashing axe-core doesn't seem like the right solution here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because I want to know when our custom grid logic fails, and it shows there's an issue that needs to be fixed. Without throwing, color contrast would hide issues that should be fixed.

@straker straker dismissed WilcoFiers’s stale review October 6, 2021 14:31

Resolved private

@straker straker merged commit 699697b into develop Oct 6, 2021
@straker straker deleted the color-contrast-empty-cells branch October 6, 2021 16:15
straker added a commit that referenced this pull request Oct 18, 2021
…nding size (#3186)

* fix(color-contrast): account for elements that do not fill entire bounding size

* not private
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.

2 participants