Skip to content
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

Merged
merged 6 commits into from Oct 21, 2021
Merged

feat: annotator component #227 #737

merged 6 commits into from Oct 21, 2021

Conversation

mturoci
Copy link
Collaborator

@mturoci mturoci commented Apr 15, 2021

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:

  • The general pattern is to split all the text strings into separate spans so that it is possible to determine start/end indeces and highlight the correct part. Also considered a simpler solution via replaceAll on string, but it would replace every occurrence in the text which may not be desired. Open for discussion

The component a touch from a designer to look better.

Demo:

Screen.Recording.2021-04-15.at.11.03.54.AM.mov

Closes #227

@mturoci mturoci requested a review from lo5 as a code owner April 15, 2021 08:56
@mturoci mturoci requested a review from geomodular April 15, 2021 08:56
@shanaka-rajitha
Copy link

@lo5 @mturoci I have updated few design samples based on the initial completed design with minor variations. Design URL

text annotator layout

@mturoci
Copy link
Collaborator Author

mturoci commented Apr 26, 2021

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 form_card.

@lo5
Copy link
Member

lo5 commented Apr 26, 2021

@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.

@shanaka-rajitha
Copy link

@lo5 @mturoci, Iv updated the annotator design based on feedback and comments. However I feel we can add more line height to allow some breathing space. With current themeing line height is restricted to the font size used.

Design URL

text annotator layout

@lo5
Copy link
Member

lo5 commented Apr 29, 2021

leftright

If we go with the right-aligned 'x' button strategy, the 'x' will end up in the lower-middle [1] if the text wraps.

[2] is not affected by word-wrap.

Also the 'x' buttons in the design look extra-small - the 'x' is not legible, and requires more precision to click on.

@shanaka-rajitha
Copy link

@mturoci I have updated the designs based on the feedback. Following items were improved

  • Close icon size increased from 9px to 16px.
  • Highlighted text will have a different font color, this is to allow font visibility based on the background.
  • Font weight changed to 300 for improved clarity and breathing space.

@mturoci
Copy link
Collaborator Author

mturoci commented Apr 30, 2021

Changed accroding to design:

Screen.Recording.2021-04-30.at.12.43.05.PM.mov
  • Close icon size increased to 18px - XD seems to render icons weirdly, it should be font based, but it seems it has a container (black circle) and text with it.
  • Nit: Maybe we could be consistent in border-radiuses - same for header and highlighted text.
  • Changed UX of removing to only appear on hover.
  • Used extra line-height for content (2) so that rows don't overlap.

@mturoci mturoci force-pushed the feat/issue-227 branch 5 times, most recently from 303e4e3 to 3b4c312 Compare April 30, 2021 11:14
@shanaka-rajitha
Copy link

Looks good @mturoci 💯

  • we can make the shapes of the tags and highlight section similar, refer attached image which shows first design with 4px radius and the second with no radius.
  • the shadow on tags is it so show selected state.

Screenshot 2021-04-30 at 18 17 04

@lo5
Copy link
Member

lo5 commented Apr 30, 2021

Observations:

  • It's odd that the word-spacing increases when you highlight text.
  • The selected-state for tags needs design @shanaka-rajitha (not sure if the drop shadow is the best mechanism here - any alternatives?)

@mturoci
Copy link
Collaborator Author

mturoci commented Apr 30, 2021

It's odd that the word-spacing increases when you highlight text.

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.

@lo5
Copy link
Member

lo5 commented Apr 30, 2021

caused by padding

Padding around the highlighted section is fine. Padding around individual words within the section is not.

@mturoci
Copy link
Collaborator Author

mturoci commented Apr 30, 2021

Padding fixed, border radius changed to 2px - same as Fluent uses.

Screen.Recording.2021-04-30.at.7.37.18.PM.mov

@lo5 lo5 added this to the 2021 milestone Apr 30, 2021
@shanaka-rajitha
Copy link

@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.

selected state of tags UI

@lo5
Copy link
Member

lo5 commented May 3, 2021

How about:

  1. Gray out (or fade out?) other tags when a tag is active.
  2. Make hover state = normal state for grayed-out tags.

@shanaka-rajitha
Copy link

@lo5 fading out is a good approach

  1. On active state other tags will fade in color or be greyed.
  2. When hovered over a a faded tag it will show the default color.

Tag active state-v2

Tag active state-v1

@mturoci
Copy link
Collaborator Author

mturoci commented May 11, 2021

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

@lo5
Copy link
Member

lo5 commented May 13, 2021

2021-05-13_10-39

  • Per design, height of tags [1] needs to be smaller, or match the height of the tagged text. Currently it's taller than a button [2].
  • [3] [4] compare left vs right padding when you select text.
  • [5] [6] looks like border-radius is missing?
  • [7] Fade/dim needs to be more pronounced (see design).

@mturoci
Copy link
Collaborator Author

mturoci commented May 14, 2021

Redesigned according the spec.

Per design, height of tags [1] needs to be smaller, or match the height of the tagged text. Currently it's taller than a button [2].

Not sure where this came from. In the design spec, the tag height is 32px, content line height 25px and submit button 42px (Fluent has button height 32px). I have fixed it according to design spec with mentioned values with the exception of Fluent button.

[3] [4] compare left vs right padding when you select text.

Fixed.

[5] [6] looks like border-radius is missing?

Fixed.

[7] Fade/dim needs to be more pronounced (see design).

IMO the proposed design with gray color is not ideal. It uses neutralLight color, which might not be a good fit with different theme (right now it looks good because it resembles disabled state, but that may not be the case with other themes). Secondly, even the current contrast between background and button text is pretty low, making it hard to read. I consider previous opacity reduction better.

Also some design spacing values are a bit strange:

  • tagged text - padding: left 11px, right 10px, top 1px, bottom 2px
  • tag padding: top 6px, bottom 5px

Result:
image

@mturoci
Copy link
Collaborator Author

mturoci commented Sep 22, 2021

@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 text_annotator to allow for multiple types of annotators in future, e.g. image_annotator. wdyt?

@lo5
Copy link
Member

lo5 commented Sep 22, 2021

I suggest renaming this component to text_annotator

Agree

@shanaka-rajitha
Copy link

shanaka-rajitha commented Oct 11, 2021

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?

@mturoci will update the following component with minor changes to match the new design guidelines.

@shihan007
Copy link

shihan007 commented Oct 13, 2021

@mturoci https://xd.adobe.com/view/0790f950-abbd-41fa-b372-332295fd876f-52c3/screen/78995835-9119-4777-b4db-b7f5dd0e7bf6
Screenshot 2021-10-13 at 12 16 21

light theme look
Screenshot 2021-10-13 at 12 19 23

@shihan007
Copy link

shihan007 commented Oct 13, 2021

Outer border to indicate selected tag is better than gray out the unselected one based on two points.

  1. Since we are using colors to indicate each label gray one also act like one of them and user may get confused which one is the selected one.
  2. Some visual impaired may find it difficult because they may see same color from both tags

Screenshot 2021-10-13 at 14 45 32

Screenshot 2021-10-13 at 14 45 22

@shihan007
Copy link

leftright

If we go with the right-aligned 'x' button strategy, the 'x' will end up in the lower-middle [1] if the text wraps.

same issue occur if we have highlighted the top text. Btw I am okay with placing it right @mturoci @lo5

cc: @shanaka-rajitha

@mturoci
Copy link
Collaborator Author

mturoci commented Oct 13, 2021

Changed according to new designs

image
image
image

API changes

  • renamed to text_annotator
  • added title prop

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 label_color to give people control over this one.

@shihan007
Copy link

@mturoci yeah this looks good. @lo5 do you think 14px font size not working for this component?

@mturoci mturoci merged commit d7b4add into master Oct 21, 2021
@mturoci mturoci deleted the feat/issue-227 branch October 21, 2021 07:51
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.

Add text annotation component to API
4 participants