search

carbon-design-system / ibm-products

A Carbon-powered React component library for IBM Products
https://carbon-for-ibm-products.netlify.app/
Apache License 2.0
87 stars 120 forks source link

feat(decorator): support second button in round-shaped decorator #2772

Closed liunate closed 10 months ago

liunate commented 1 year ago

Contributes to #2750 - decorator with enhanced accessibility

What did you change?

  1. Add one more decorator variants with a second user interaction interface <button>, which could play the same role as pointer-right-click(onContextMenu).
  2. Round shaped decorator. It can replace deprecated <Pill>.

How did you test and verify your work?

  1. Keep original jest test cases with additional tests on the 2nd <button>.
  2. Keep original storybook stories while adding some new stories showcase 2nd <button>.
liunate commented 1 year ago

Hello 👋 while I am migrating the new decorator code base, I am wondering whether we need some feature flag for people don't want the new round-shaped decorator? Like in this style file line some overriding rules making decorator border looks round followed. Will it be a good idea to leave the option to consumers whether they want to move over to new decorator look, or stay at original square-shaped decorator?

FYI you can see new decorator in action by spinning up storybook and go to:

iframe.html?knob-Active (`active`)=true&knob-Link (`href`)=&args=&id=decorator--default

I notice there are couple of components leveraging <Decorator> like DataDecorator, Pill,

netlify[bot] commented 1 year ago

Deploy Preview for carbon-for-ibm-products ready!

Name Link
Latest commit f890b86a8e984eed46cc0ecabf4ab04b96100c55
Latest deploy log https://app.netlify.com/sites/carbon-for-ibm-products/deploys/6421a71c75750a000848d7b0
Deploy Preview https://deploy-preview-2772--carbon-for-ibm-products.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

liunate commented 1 year ago

@amtoussam @jlongshore I just checked up on the PR status and I'm wondering if there's anything else I can help to progress this PR? Tks~

amtoussam commented 1 year ago

@liunate There are conversations designers are having with each other but this implementation. I hope to have an answer for you by next week.

CC: @jlongshore

bobbinrobyn commented 1 year ago

@amtoussam - have you synced with Jamie, Cameron, or whoever else needed to be involved? I know Jamie is planning to do a design review ASAP.

amtoussam commented 1 year ago

@jgodman Please review this PR and let us know if it meets Design standards.

jgodin2023 commented 1 year ago
  1. The decorator should not have an underline on hover, as per HTML standards this is not a link as it opens the right side panel after clicking it.

https://user-images.githubusercontent.com/130777697/232563127-cdc12402-0667-41c0-9bc3-6f0b1cea9c69.mov

  1. I didn't see if the left-click context menu was available in the repo.

Screenshot 2023-04-17 at 1 24 35 PM

liunate commented 1 year ago
  1. decorator should not have an underline on hover

The underline is only drawn when the right part of decorator alone is clickable to match existing decorator behavior and the new purposed design in figma

Do we want to change that to better match HTML standard?

liunate commented 1 year ago
  1. I didn't see if the left-click context menu was available in the repo.

There are two reasons it's not part of this PR:

  1. Carbon haven't had their Menu component ready for production(still in experiment).
  2. I saw various decorator usages: with or without context menu(by various libraries). It seems to me decorator could be one general component that we don't force the context menu, or that context menu could be attached by decorator consumer by their own needs.
jgodin2023 commented 1 year ago
  1. decorator should not have an underline on hover

The underline is only drawn when the right part of decorator alone is clickable to match existing decorator behavior and the new purposed design in figma

Do we want to change that to better match HTML standard?

Hey Nate, yes! Design has given this some thought as we want to move toward links being navigational, so removing the underline on hover and click will work well in this case.

jgodin2023 commented 1 year ago

2. by various libraries).

Hmm, Interesting, the reason we split the decorator in two was to have the left side have an action, the action was to open the context menu and provide a way to "add to case" etc. Im not sure I understand, if we don't have a context menu showing when the user clicks the left side of the decorator button, what happens?

liunate commented 1 year ago

if we don't have a context menu showing when the user clicks the left side of the decorator button, what happens?

Current implementation leaves the decision to new decorator consumer. The reason is mentioned here.

The biggest obstacle here is we don't have a context menu in shared code base. Current decorator users are using their own menu implementation(home-brewed, external libraries). By leaving the decision of what to do at click to them, they can keep their current implementation. It's also when Carbon menu is ready for production, we could enhance new decorator with that to provide a built-in menu feature on new decorator.

amtoussam commented 1 year ago

@jgodin2023

elycheea commented 11 months ago

@liunate Any updates on this progress?

liunate commented 11 months ago

hi @elycheea sorry for the delay, I was busy with another assignment but will definitely progress this 1st thing next week.

liunate commented 11 months ago

What I plan to do is:

  1. Rebase to keep good base of this PR head branch
  2. Remove the underline per previous design comment
  3. Check back here again see if we could move on next stage

As a side note, I just reviewed latest carbon menu and it is now production ready. That menu could be the next thing we could include in this component given that context menu is the common decorator use case.

netlify[bot] commented 11 months ago

Deploy Preview for v1-carbon-for-ibm-products ready!

Name Link
Latest commit 31aa9a015f027f39f7b32d7c6a6a586d58e671ec
Latest deploy log https://app.netlify.com/sites/v1-carbon-for-ibm-products/deploys/64c3c2ddeaf1e600073644f1
Deploy Preview https://deploy-preview-2772--v1-carbon-for-ibm-products.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

liunate commented 11 months ago

What I plan to do is:

  1. Rebase to keep good base of this PR head branch
  2. Remove the underline per previous design comment
  3. Check back here again see if we could move on next stage

@elycheea @jgodin2023 I just rebased and updated the component by previous comment. Let me know if there's anything else needs to be done to proceed with this PR. Many thanks!

elycheea commented 11 months ago

@jgodin2023 I’m running the checks now so once they all pass, there should be a new preview if you could re-review. @jlongshore and I can help with the review from here. Thank you!

liunate commented 11 months ago

@elycheea tks for triggering the CI run for me and it's obvious I missed some linting and snapshot things, so I just fixed them right away. What's still blocking is I saw there are couple a11y test failures from c4p that does not seem to be related in any way to the security Decorator I am working on. These three c4p test cases all failed at the a11y:

@carbon/ibm-products:  FAIL  src/components/EditSidePanel/EditSidePanel.test.js (14.743 s)
@carbon/ibm-products:   ● EditSidePanel › has no accessibility violations

@carbon/ibm-products:  FAIL  src/components/CreateSidePanel/CreateSidePanel.test.js (15.734 s)
@carbon/ibm-products:   ● CreateSidePanel › has no accessibility violations

@carbon/ibm-products:  FAIL  src/components/APIKeyModal/APIKeyModal.test.js (18.969 s)
@carbon/ibm-products:   ● APIKeyModal › has no accessibility violations

I can reproduce the same locally by running yarn run ci-check:test:c4p. Then I logged the rendered DOM to another browser tab and ran DevTool IBM achecker, it shows me the same violation as above.

    await wait(3000);
    screen.logTestingPlaygroundURL();

I know at this moment these packages c4p and security are configured independent versioning, and c4p could only actively maintained at another branch main instead of main_v1. Could that be the reason over time package c4p on main_v1 generating more a11y violation as accessibility check rule updated? Or are those a11y or whatever test fails in that c4p package not something we care about for this PR head branch based on main_v1?

elycheea commented 11 months ago

@liunate I can help look into the that — I think it’s this issue that we fixed in main (#3177) but might not have opened a PR for main_v1 yet.

I think better to keep it as a separate PR that we merge before this one so you can keep the PR scope small here. Will let you know once we address it!

elycheea commented 11 months ago

We merged https://github.com/carbon-design-system/ibm-products/pull/3233 this morning — this should resolve the issue now. 🤞

liunate commented 11 months ago

We merged #3233 this morning — this should resolve the issue now. 🤞

@elycheea Thanks for the prompt fix 🙏 Just ran all tests locally again and all passed, so could you please help trigger another round of CI check?

liunate commented 10 months ago

Hello there~ just checking in and wondering if there's anything else needed to move forward with this PR? Tks~