Conversation
tzi
added
the
Pull Request WIP
tzi
changed the title
tzi
force-pushed
the
ux-326
branch
10 times, most recently
from
24a8707 to
cb064c5
Compare
There was a problem hiding this comment.
ℹ️ The new tests changed the visible focused element on this screenshot.
There was a problem hiding this comment.
ℹ️ The starred segment comes at the top of the list
| self.target.find('[title]').each(function () { | ||
| addTooltip(this, this.getAttribute('title')); | ||
| }); |
There was a problem hiding this comment.
ℹ️ Fix tooltip for all actions in the SegmentList
| display: flex; | ||
| flex-direction: column; |
There was a problem hiding this comment.
ℹ️ Use flexbox to be able to reorder starred segment easily.
| .segmentationContainer .submenu ul li[data-idsegment=""] { | ||
| order: 0; | ||
| } |
There was a problem hiding this comment.
ℹ️ First line is always "All visitors"
tzi
force-pushed
the
ux-326
branch
2 times, most recently
from
d004753 to
5c02c19
Compare
tzi
added
Enhancement
tzi
force-pushed
the
ux-326
branch
5 times, most recently
from
8172f5b to
bac2467
Compare
| var comparisonService = window.CoreHome.ComparisonsStoreInstance; | ||
| comparisonService.addSegmentComparison({ | ||
| segment: $(e.target).closest('li').data('definition'), | ||
| const $button = $(this); |
There was a problem hiding this comment.
Small issue, but is there a reason you are using $button instead of button?
There was a problem hiding this comment.
Do you mean the name of the variable?
I try to prefix the variable that are a jQuery element with $, I think I’ve seen this on the repository already. Because a button and a $button will not have the same API. So I know if I should write $button.getAttribute('data-state') or $button.attr('data-state').
| const starTitleAttribute = 'title="' + getStarSegmentTitle(segment, canEdit) + '"'; | ||
| listHtml += '' + | ||
| '<button data-star class="starSegment" '+ starTitleAttribute + ' ' + disabledAttribute + '>️' + | ||
| '<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 24 24">' + |
There was a problem hiding this comment.
Shouldn't we try to avoid inline svg's in general for the sake of re-usability and easy replaceability?
I understood that we can't use a font icon, due to the animation. But would that be possible through a background image maybe? That way we could handle that with a CSS class only, which would allow themes to overwrite it more easily 🤔
There was a problem hiding this comment.
I choose to not use a background image here, since we will not be able to change it’s color easily.
Otherwise we could a colored svg and make it black by default with a filter 🤔
There was a problem hiding this comment.
Discuss directly. It could be done in a follow up PR 👍
6 participants
Description
New “Star” Feature: each segment can be "starred" by clicking on a star icon.
Starred segments are highlighted at the top of the list.
What happen when a segment star is clicked :
Screenshots
before / after
tooltips
Animation
Checklist
Review