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

Additional tag features #32

Closed
2 tasks done
SimonFinney opened this issue Jul 28, 2020 · 19 comments
Closed
2 tasks done

Additional tag features #32

SimonFinney opened this issue Jul 28, 2020 · 19 comments
Assignees
Labels
Epic status: needs release review 👀 Component is ready for release review

Comments

@SimonFinney
Copy link
Contributor

SimonFinney commented Jul 28, 2020

View additional tags implementation

To do

@carrenelloyd
Copy link

carrenelloyd commented Jul 30, 2020

Link to design

Maintainer:
Marcie Coral
Courtney Banning

Catalog number: 1

@SimonFinney
Copy link
Contributor Author

@carrenelloyd Would you know if there's redlines or existing code available for this?

@lee-chase lee-chase self-assigned this Sep 16, 2020
@lee-chase
Copy link
Member

@marciecorral initial dev review.

  1. In essence, the overflow part is a tooltip. The only difference is the change to Gray90
  2. Gray90 is not a Carbon theme token and should NOT be used see https://www.carbondesignsystem.com/guidelines/color/usage
    $interactive-02: Gray 80 is a better choice as the current tooltip background and the high contrast tooltip.
  3. Hover is a poor interaction which does not work on well for interactive elements such as this one. Carbon defines the clickable interactive tooltip for this scenario.
  4. What happens after the modal is closed?
    4.1. A keyboard user may expect the focus to return to the +tag or list.
    4.2. Is the list still open?
  5. Should it support the tooltip positional settings?
  6. The modal is ugly, would it not be better to present the tags as tags in this list. These could be presented many to a line.
  7. If there are enough to cause the modal to scroll should it be filterable?
  8. How narrow should it go? Should we assume it may not even fit the first tag https://codesandbox.io/s/brave-joliot-expkd
  9. It is not true that accessibility is unchanged we need to ensure we deal with hidden information.
  10. $link-01 is blue 60 and this color should be used for the link as it is the default carbon color.

Developer notes: The Carbon tabs now implements a feature where it assesses whether enough space is available. The same technique may be applicable here.

@marciecorral
Copy link

This is an item that was designed in 2019 by a designer who is no longer here at IBM. This was never implemented in CP4MCM. We can discuss details on the call tomorrow.

@Levinson1
Copy link

@lee-chase
Copy link
Member

lee-chase commented Sep 17, 2020

@Levinson1 there are open questions that are not addressed in the InVision prototype.

@ghost
Copy link

ghost commented Sep 18, 2020

@lee-chase

  1. Yes, this is essentially a tool tip. However my file is showing the tool tip to be Gray 80 (#393939).
  2. and 3. I am not sure where the disconnect is happening, but my file for this component is showing the tool tip and the tag on hover as Gray 80 or #393939.
  3. The original designer and myself thought that hover would be the best interaction for this situation. Why do you believe this interaction is not the best option?
  4. Not sure what you're asking. Could you clarify? The modal will work like a typical modal.
  5. When the modal is closed, the focus will return to the "View all labels" link within the tool tip.
  6. When the modal is closed, the tool tip is still open and the focus returns to the last place of focus before the modal was opened. Which in this case, is the "View all labels" link.
  7. Can you clarify?
  8. The decision to have the tags in a list was because some labels can be too long for a tag and so we use an ellipses. With this list view there are no excessively long tags.
  9. Do you mean, should we include a search bar in the modal? The tags in the modal are read only with no capabilities for selecting.
  10. The Additional Tag should only be as narrow as the plus sign and number within (ex. +1). Any other tags should only be as long as the label within.
  11. I addressed your feedback about accessibility in points 6 and 7. Is there another issue of accessibility you are referring to?
  12. Since the link text is on Gray 80 (#393939), I used the link component from Gray 90 theme. The link text should be Blue 40 or #78A9FF in order to meet accessibility.

@lee-chase
Copy link
Member

@CourtneyBanning

  1. & 2. It may be the case that gray 90 was used at some point. The carbon themes for Gray 90 and Gray 100 do use Gray 90 but this is not currently used in White and Gray 10 themes.

  2. Hover does not move focus, unlike click/touch. As a result, accessing the interactive part of the element via the keyboard is hard to achieve. Carbon uses focus for all overflow menu and interactive tooltip scenarios.
    4., 5 & 6 we seem to be mixing numbers and you have answered 4 for me.

  3. (My 5) Tooltips support positions: top, left, bottom, right and alignment of start, centre, end. Should component support these?

  4. (My 6) Yes, but the modal shows plain text instead of tags which would be I think more aesthetically pleasing. I'm not a visual designer so feel free to shoot me down on this. Another option is to use the Carbon list component.

  5. (My 7) Yes just thought based on the list appearing to be scrollable in the design (fades out towards bottom).

  6. (My 8) Agreed but is there a minimum width for the set of tabs? If the first tag is 200px wide and we only have 200px for instance what doe we display?

  7. (My 9) Yes, sorry for not being clear or polite ;-). The tooltip (non-interactive) provides the assistive text for screen readers, the interactive tooltip has a label. At the very least we need to have assistive text along the lines of "Additional tag list" or "Shows 5 additional tags"

  8. You are correct this is represented by $inverse-link in white and gray 10 themes.

  9. Bonus question. Tags can be different colors, the design only shows the high contrast which can be seen on https://react.carbondesignsystem.com/?path=/story/tag--default by changing the setting in the 'Knobs' tab. Is it OK to assume the additional tags tag uses high-contrast.

  10. Bonus question 2. The standard Tag component has a filter property, is this permitted in sets of tags used in this way?

P.S. I believe we should try to stick to Carbon theme color tokens where every possible as opposed to the underlying Carbon colors. https://www.carbondesignsystem.com/guidelines/color/usage

@ghost
Copy link

ghost commented Sep 24, 2020

@lee-chase

Sorry about the delay in response! You brought up some great points and I am re-evaluating aspects of this component. I will get back to you as soon as it is sorted.

@lee-chase
Copy link
Member

Peter Mills (IBM) posted an existing implementation example on internal slack. He is investigating the possibility of open sourcing the component.

https://files.slack.com/files-pri/T27SFGS2W-F01DE57RS3H/action_tag_list.gif

@lee-chase lee-chase removed the on HOLD label Nov 13, 2020
@lee-chase
Copy link
Member

NOTE:

@lee-chase
Copy link
Member

Updates required by #59

@ghost
Copy link

ghost commented Nov 19, 2020

Just updated the original issue with updates to the pattern.

https://github.ibm.com/CloudIntegrationDesign/DesignSystemAdoptionGuild/issues/92

@lee-chase many of your concerns are addressed with this updated version. thanks for the feedback!

@lee-chase
Copy link
Member

@CourtneyBanning the current design extracts text from the tag, which is technically problematic due to the way React works and Carbon tags not requiring text.

The following is valid

    <Tag type="red" title="Clear Filter">
      <Warning16 />
      <Warning16 />
      <Warning16 />
      <Warning16 />
    </Tag>

The design also precludes the use of filter tags.

This implementation https://ibm-cloud-cognitive.netlify.app/?path=/story/experimental-tagset--many-tags produces the following

image

image

image

@lee-chase
Copy link
Member

@CourtneyBanning please review the current tags with overflow implementation. It currently does not display a simple text in the overflow pop up as this is not compatible with the tag implementation. We could use the contents of the tag but that can be anything, such as an icon.

Note 1: The next version of Carbon is required to align the popup, which should be in the next few days.
Note 2: There appears to be an issue in Carbons floating menu positioning. I will confirm on next release and raise a bug.

https://ibm-cloud-cognitive.netlify.app/?path=/story/experimental-tagset--many-tags

@lee-chase
Copy link
Member

@CourtneyBanning has asked the overflow is changed to reflect the content of the tags, which will likely be text.

Noting that the design will need to document; Text only (Carbon allows anything), filter and disabled should not be used.

@lee-chase
Copy link
Member

Shared by @CourtneyBanning in slack.

image

Requested clarification/noted in slack by @lee-chase
Standard Carbon modal widths are 24%, 36%, 48%, and 72%.
Do we really want a width of 640px when space is available? 640 is approx 38% width of a macbook pro screen or 36% which is the 'md' sized modal. It is worth noting that the Create Modal which uses size 36%.

@lee-chase
Copy link
Member

lee-chase commented May 21, 2021

md: 36% width agreed with @CourtneyBanning on slack.

@lee-chase
Copy link
Member

@courtney-banning has given her approval of the implementation with PR #774

Note: Courtney has asked for the above github handle to be used.

@lee-chase lee-chase removed the status: needs design review 🎨 Component is ready for design review label May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic status: needs release review 👀 Component is ready for release review
Projects
None yet
Development

No branches or pull requests

7 participants