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: adding the ability to disable modals for TagSet component #5753

Merged

Conversation

jeesonjohnson
Copy link
Contributor

@jeesonjohnson jeesonjohnson commented Jul 29, 2024

Closes #5726

Added the ability to disable the overflow modal for additional tags. This gives consumers the customisability to use their component to manage and view tags.

What did you change?

packages/ibm-products/src/components/TagSet/TagSet.stories.jsx
packages/ibm-products/src/components/TagSet/TagSet.test.js
packages/ibm-products/src/components/TagSet/TagSet.tsx
packages/ibm-products/src/components/TagSet/TagSetOverflow.tsx

How did you test and verify your work?

Added additional unit tests

Visual testing:

Screen.Recording.2024-07-29.at.14.18.06.mov

@jeesonjohnson jeesonjohnson requested a review from a team as a code owner July 29, 2024 13:19
@jeesonjohnson jeesonjohnson requested review from devadula-nandan and annawen1 and removed request for a team July 29, 2024 13:19
Copy link
Contributor

github-actions bot commented Jul 29, 2024

DCO Assistant Lite bot All contributors have signed the DCO.

Copy link

netlify bot commented Jul 29, 2024

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

Name Link
🔨 Latest commit cb89c21
🔍 Latest deploy log https://app.netlify.com/sites/carbon-for-ibm-products/deploys/6707bac550187d00096e1ccb
😎 Deploy Preview https://deploy-preview-5753--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.

@jeesonjohnson
Copy link
Contributor Author

I have read the DCO document and I hereby sign the DCO.

@jeesonjohnson
Copy link
Contributor Author

recheck

Copy link
Contributor

@vladbalanescu vladbalanescu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@matthewgallo
Copy link
Member

Hey @jeesonjohnson, someone will review this PR after the team has triaged the related issue (#5726). Thanks for your patience! 😄

@jeesonjohnson
Copy link
Contributor Author

Sounds good, thanks for the update @matthewgallo!

@devadula-nandan devadula-nandan added the needs: design opinion Design question needs opinion from designer label Aug 7, 2024
@davidmenendez
Copy link
Contributor

@jeesonjohnson can you please resolve the merge conflicts? thanks!

@jeesonjohnson
Copy link
Contributor Author

Hey @davidmenendez, gone ahead and resolved the merge conflicts.

@devadula-nandan devadula-nandan removed the needs: design opinion Design question needs opinion from designer label Sep 20, 2024
@devadula-nandan
Copy link
Contributor

Hey @jeesonjohnson,

After these changes, I am wondering how you could set the onClick on the overflow tag to show your custom modal.

You still need to access the dom directly, get the overflow tag, and attach an onClick event listener. which isn't a good approach in a react-handled application. also, it could be a mess targeting a tagset if there are multiple of them on the page.

instead of introducing disablePopOver we could introduce something like onOverflowClick like below

<TagSet tags={tags} onOverflowClick={customOverflowHandler} />

and in TagSetOverflow.jsx

...
  const handleOverflowClick = () => {
    if (onOverflowClick) {
      onOverflowClick(overflowTags);  // Invoke the custom handler
    } else {
     setPopoverOpen?.(!popoverOpen) // Opens default popover
    }
  };
...
   <Tag onClick={handleOverflowClick} className={cx(`${blockClass}__popover-trigger`)} >
...

Now if onOverflowClick is not passed, we can see the default behavior, and if passed, will invoke the custom handler.
which would give better control to the user.

cc: @davidmenendez any second thoughts/suggestions on this?

@jeesonjohnson
Copy link
Contributor Author

jeesonjohnson commented Oct 4, 2024

Hey @davidmenendez!
Thanks for the suggestion, I think that makes a lot of sense, and I have updated the PR accordingly. It should strike a balance between flexibility and solving the issue.

Here is a quick video of the above feature working as expected:

main2.mov

@devadula-nandan
Copy link
Contributor

@jeesonjohnson can you fix the spell checks?

@jeesonjohnson
Copy link
Contributor Author

@davidmenendez Done! :)

Copy link
Contributor

@davidmenendez davidmenendez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for all your hard work! just a few things i noticed

Copy link

netlify bot commented Oct 10, 2024

Deploy Preview for ibm-products-web-components canceled.

Name Link
🔨 Latest commit cb89c21
🔍 Latest deploy log https://app.netlify.com/sites/ibm-products-web-components/deploys/6707bac552d7a60008f5ec5c

@jeesonjohnson
Copy link
Contributor Author

Thanks for @davidmenendez . I have gone ahead and addressed all the comments. Mind having another look please?

Copy link
Contributor

@davidmenendez davidmenendez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@devadula-nandan devadula-nandan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@devadula-nandan devadula-nandan added this pull request to the merge queue Oct 11, 2024
Merged via the queue into carbon-design-system:main with commit 29e960c Oct 11, 2024
26 checks passed
@jeesonjohnson jeesonjohnson deleted the tag_set_overflow_disable branch October 11, 2024 12:02
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.

[Feature Request]: Disable Modal for TagSet component
5 participants