Update media cell selection position #205
Conversation
…ition # Conflicts: # Pod/Classes/WPMediaCollectionViewCell.m
frosty
left a comment
frosty
left a comment
There was a problem hiding this comment.
So I thought performance might be an issue, but it actually seems fine! One improvement could be to only update the actual selected state if it's changed. We could also potentially set a shadowPath on the position label's layer.
Left a couple of comments about some of the styles not matching the style document.
Also, whilst it's an unlikely situation I think we should probably do something about the case when the number of selected items is longer than 2 digits. Should let the label grow?
| } else { | ||
|
|
||
| _positionLabel.text = @""; | ||
| _positionLabel.backgroundColor = [[UIColor lightGrayColor] colorWithAlphaComponent:0.7]; |
There was a problem hiding this comment.
This colour doesn't match the style guide – 198, 198, 198, 0.3
There was a problem hiding this comment.
Good catch, I think I'm going to make this color configurable trough an appearance method to make it easier to configure for other users of the library.
| } else { | ||
| self.positionLabel.hidden = YES; | ||
| } | ||
| [self updatePositionLabelToSelectedState:self.isSelected]; |
There was a problem hiding this comment.
We could only update the label if the selected state has actually changed?
| if (selected) { | ||
| _positionLabel.backgroundColor = [self tintColor]; | ||
| _positionLabel.layer.borderColor = [self tintColor].CGColor; | ||
| _positionLabel.layer.shadowColor = [UIColor colorWithWhite:0 alpha:1].CGColor; |
There was a problem hiding this comment.
I don't think the shadow colour or position (shouldn't be an offset?) matches the style guide here. But we should also wait for @iamthomasbishop's input...
There was a problem hiding this comment.
I need to remove that shadowColor just realize that shadow is applied for the text on the label , and not for the border/circle.
|
@frosty ready for another round. |
|
@iamthomasbishop updated screenshot with shadows on selection.
|
I think 10pt from each edge looks good. I can't imagine there are any cases of videos having more than 5 digits, so the space should be sufficient. Noticed you updated the shadow on selection dot 👍 |
Good call. The label's font size should probably shrink in those cases to fit 3 digits, even if the fit is tight. The font size here can be 10pt. Example:
@SergioEstevao Another thing I noticed. Is the label set in Regular weight or is just my eyes playing tricks on me? It should be Medium. |
|
@iamthomasbishop and @frosty I setup the position to label to auto shrink it looks better but because of the circle mask applied to the label I see some numbers being cropped.
|
Looking good! Maybe we could come down 1pt more, to 9pt? I know that's really small, but I think this is appropriate for such an edge case (user selecting 100+ media items at a time). |
| _position = position; | ||
| if (position != NSNotFound) { | ||
| CGFloat fontSize = position < 100 ? 13.0 : 9.0; | ||
| _positionLabel.font = [UIFont systemFontOfSize:fontSize weight:UIFontWeightMedium]; |
There was a problem hiding this comment.
Could we pull the font sizes and weight out into constants so we only need to change them in one place? (we're setting 13 and medium in 2 different places)
I blame @jleandroperez for my pickiness about these things, sorry :)
There was a problem hiding this comment.
@frosty Done! Just don't ask me to right align variable values :)
SergioEstevao
deleted the
issue/204_update_media_cell_selection_position
branch
Fixes #204
This PR updates the appearance of the cell image selector/position to the new design.
It now looks like this:
To test:
@iamthomasbishop: Do you think the 10pt distance from the edge looks ok? Do you think we need the shadow on the position button? what about to always keep the white border?
@frosty, performance wise to you the cornerRadius option is making things slow while scrolling? I can give a try to shapeLayer if you think it's too slow.