Added highlight to all blocks within same range#268
Added highlight to all blocks within same range#268priley86 merged 1 commit intopatternfly:masterfrom
Conversation
| var fillColor; | ||
| blocks.call(highlightBlock, false); | ||
| d3.select(this).call(highlightBlock, true); | ||
| if (scope.maxSize < 16) { |
There was a problem hiding this comment.
This should be blockSize (computed) not scope.maxSize.
There was a problem hiding this comment.
16 might be a good default, but the application should be able to set that value if desired.
There was a problem hiding this comment.
FYI, I added an attribute to override the default value. However, I'm not using blockSize because that adds padding. When the maxBlockSize attribute is 15, blockSize equals 20.
There was a problem hiding this comment.
But scope.maxSize is passed in, it does not mean that is the size of the blocks. For example, an application may want a maximum block size of 100. That means that even if there are only 2 data items, only use 100x100px blocks, do not fill the entire area with huge blocks. We set the max block size small in these examples to show small blocks.
If I set a maxSize of 100 but there are 10,000,000 blocks, the computed block size will be small and THAT is when we want to use this functionality.
There was a problem hiding this comment.
BTW, in that method (redraw) you can use fillSize if you want the size of the block without padding.
There was a problem hiding this comment.
Thanks for clarifying. What I really want to test is blockSize - scope.padding (i.e., the fillSize var). It wasn't correct having to set max-block-size="13" in order for blockSize to be less than 16 and highlighting finally becomes enabled.
dlabrecq
force-pushed
the
PTNFLY-1226
branch
2 times, most recently
from
3627231 to
1761d6c
Compare
|
Added tooltips specific to range and met with @LHinson to review.
|
| }; | ||
|
|
||
| // Highlight range on hover when default > fill size or when rangeOnHover is true | ||
| var highlightRange = ((scope.rangeOnHover === undefined && fillSize < 16) || scope.rangeOnHover) ? true : false; |
There was a problem hiding this comment.
I believe that 16 is a good default but should be customizable by the application
dlabrecq
force-pushed
the
PTNFLY-1226
branch
2 times, most recently
from
59ce3a9 to
a70218c
Compare
|
@jeff-phillips-18 I added the minBlockSize and rangeHoverSize attributes per our discussion. The rangeOnHover=false will still disable the feature. |
|
|
||
| //Allow overriding of defaults | ||
| if ($scope.minBlockSize === undefined || isNaN($scope.minBlockSize)) { | ||
| $scope.minSize = 5; |
There was a problem hiding this comment.
The design spec says "Recommended: Minimum block size is 2x2"
There was a problem hiding this comment.
I don't see scope.minBlockSize being used anywhere. After the call to determineBlockSize, the maxBlockSize is checked but I don't see a minBlockSize check.
There was a problem hiding this comment.
I read this in Google doc; “In angular patternfly we set the max size to be 50px but the application can set the max for a particular heat map to be from 5-50”. I assume this means we don't want to make the default smaller than 5 for angular?
Regarding the check after determineBlockSize, we never had a test for min block size. I'll look at adding one.
There was a problem hiding this comment.
I think it is OK to have a max range of 5-50 and still set the min to 2. I believe we should follow the spec regardless of range hover being enabled.
…ss than 16x16px. Added tooltips specific to range. Added minBlockSize and rangeHoverSize attributes for overrides. PTNFLY-1226
|
Added a check of scope.minSize (i.e., minBlockSize) after calling determineBlockSize, similar to the scope.maxSize test. |
|
LGTM |
|
👍 |
3 participants
Added highlight to all blocks within same range when block size is less than 16.
PTNFLY-1226