-
Notifications
You must be signed in to change notification settings - Fork 327
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
feat: annotator component #227 #737
Conversation
@lo5 @mturoci I have updated few design samples based on the initial completed design with minor variations. Design URL |
Looks good @shanaka-rajitha! @lo5 I suppose we are going with the first (left side) option since this is going to be part of a |
@mturoci - yes I like the muted colors in the second design. I liked the position of the 'x' in Martin's implementation - allows us to show-on-hover. The inline 'x' in the second design adds to visual noise. It would be good if the highlighted text were inconspicuous. |
@mturoci I have updated the designs based on the feedback. Following items were improved
|
Changed accroding to design: Screen.Recording.2021-04-30.at.12.43.05.PM.mov
|
303e4e3
to
3b4c312
Compare
Looks good @mturoci 💯
|
Observations:
|
caused by padding - the same what prodigy does to make a better distinction beween highlighted and non-highlighted text. We can go with just background color change ofc. |
Padding around the highlighted section is fine. Padding around individual words within the section is not. |
Padding fixed, border radius changed to Screen.Recording.2021-04-30.at.7.37.18.PM.mov |
@lo5 I have shared 2 samples for selected-state of tags which is very subtle. Since the tag colors vary from each other having a color shade for active tag would not highlight the purpose. |
How about:
|
@lo5 fading out is a good approach
|
Rebased and changed active tag styling via opacity. It has full opacity on either hover or selection. Also made first tag selected by default: Screen.Recording.2021-05-11.at.11.55.52.AM.mov@shanaka-rajitha @lo5 wdyt |
Redesigned according the spec.
Not sure where this came from. In the design spec, the tag height is
Fixed.
Fixed.
IMO the proposed design with gray color is not ideal. It uses Also some design spacing values are a bit strange:
|
@shanaka-rajitha is this design still valid in respect to https://xd.adobe.com/view/0790f950-abbd-41fa-b372-332295fd876f-52c3/?x_product=cc-slack%2F1.5.1 ? If not, can you please update accordingly? @lo5 I suggest renaming this component to |
Agree |
@mturoci will update the following component with minor changes to match the new design guidelines. |
Outer border to indicate selected tag is better than gray out the unselected one based on two points.
|
same issue occur if we have highlighted the top text. Btw I am okay with placing it right @mturoci @lo5 cc: @shanaka-rajitha |
a74fb92
to
4a23d2f
Compare
Changed according to new designs API changes
text color in tags is either black or white depending on lightness of background color (https://gomakethings.com/dynamically-changing-the-text-color-based-on-background-color-contrast-with-vanilla-js/). @lo5 I am wondering whether we want to introduce |
Introduces ui annotator that works similar to highlighter - user needs to first select a tag and then pick words that he wants to have highlighted.
Implementation notes:
replaceAll
on string, but it would replace every occurrence in the text which may not be desired. Open for discussionThe component a touch from a designer to look better.
Demo:
Screen.Recording.2021-04-15.at.11.03.54.AM.mov
Closes #227