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

fix: Command/Combobox TypeError and Unclickable/Disabled items + Documentation #2945

Merged
merged 11 commits into from
Aug 5, 2024

Conversation

jhnguyen521
Copy link
Contributor

@jhnguyen521 jhnguyen521 commented Mar 8, 2024

Fixes #2944 due to latest cmdk update with breaking changes for the Command component. Until this fix is merged, all new command components that install cmdk@1.00 will be broken and items will not be selectable. People who also have existing components and update to cmdk 1.00 and don’t have a <CommandList> will experience crashes upon opening up the component. See issue above for more details.

  • Additionally handling the data-disabled property better in all components, should be backwards compatible
    ^ Happy to not do this for all other components, but I figure it's better to just do this preventatively if it's backwards compatible. Either way, things will break for data-disabled="false" for all existing data-[disabled]
  • Applied same fix for aria-selected
  • Fixing broken examples for command component

Copy link

vercel bot commented Mar 8, 2024

@jhnguyen521 is attempting to deploy a commit to the shadcn-pro Team on Vercel.

A member of the Team first needs to authorize it.

@jhnguyen521 jhnguyen521 changed the title Fix Command Component fix: Broken Command component from breaking cmdk change Mar 8, 2024
@kevinmitch14
Copy link
Contributor

kevinmitch14 commented Mar 8, 2024

This PR handles the proper styling via data attributes + dependancy bump + update registry.

#2626

@jhnguyen521
Copy link
Contributor Author

Fixed crlf line endings to lf. May be worth documenting this in contributing, but pnpm seems to output files with \r\n instead of just \r on windows machines, ignoring tsconfig compiler options

@kevinmitch14
Copy link
Contributor

The component specific issues have already been fixed in #2626 🤔

@jhnguyen521
Copy link
Contributor Author

jhnguyen521 commented Mar 11, 2024

The component specific issues have already been fixed in #2626 🤔

After you later added the bug fix from this and the other issue afterwards in a later commit, then yes if that counts as "already" :) Also, aria is already set in cmdk and we can be relatively certain that will always be there for accessibility reasons, not sure how opinionated we should be about using their aria/data attribute. Personally, I think it's fine to leave as is but what do I know.

Also, this PR updates necessary documentation to handle other breaking changes in cmdk.

@jhnguyen521
Copy link
Contributor Author

Happy to do a version bump in this PR as well.

@kevinmitch14
Copy link
Contributor

kevinmitch14 commented Mar 11, 2024

I am not too sure why you are hinting that I have "copied" your work or something?

With all due respect, the original #2626 PR was opened in January as a follow up from #1980 that was opened in November 2023. I have been monitoring this via an issue opened in the cmdk repo itself.

After my initial comment on this PR, I noticed that you made some extra commits, specifically, extra changes that were made in #2626 that had not been made in this PR. I see that these commits have now been wiped out as you cleaned the commit history...

You also commented on my PR which you have since deleted...I don't understand

After you later added the bug fix from this and the other issue afterwards in a later commit

Here is the commit you speak of...430c1b3

@jhnguyen521
Copy link
Contributor Author

jhnguyen521 commented Mar 11, 2024

After my initial comment on this PR, I noticed that you made some extra commits, specifically, extra changes that were made in #2626 that had not been made in this PR. I see that these commits have now been wiped out as you cleaned the commit history...

Yes, the change I made were applying the bug fix to the aria-selected attributes, which is the purpose of this PR? (which I might add, was also discussed and brought to my attention by someone else in #2944, so not sure what you are trying to re-imply? lol) Whereas your PR was originally just changing aria-selected -> data-selected and doing a version bump.
Huh? Cleaned the commit history? Probably because just did a rebase after some new stuff was merged in, but all commits should be as they have...

You also commented on my PR which you have since deleted...I don't understand

My comment was on being opinionated about aria-selected vs data-selected, but I realized that I was incorrect about some assumptions I was saying that made my comment irrelevant so I just deleted it :)

All I'm saying is that, it's not very cool to come in saying "already been fixed here 🤔", after applying a fix that wasn't there in the original commit an doesn't have any changes around the documentation for this component, when the purposes of our PRs are different, especially when I've been discussing with others in #2944 to workaround the issue.

Either way, I was initially annoyed, but if maintainers just want to use the earlier commit, happy to close mine and you can merge in the doc changes or I just make another PR with that. 🤷

@jhnguyen521 jhnguyen521 changed the title fix: Broken Command component from breaking cmdk change fix: Command/Combobox TypeError and Unclickable/Disabled items + Documentation Mar 12, 2024
Copy link

@CentraXx CentraXx left a comment

Choose a reason for hiding this comment

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

For many components which are not from cmdk the applied changes do not work and cause the disable state to not work anymore.
Means the fix: data-[disabled='true'] does not work for all components.

apps/www/registry/default/ui/select.tsx Outdated Show resolved Hide resolved
apps/www/registry/default/ui/dropdown-menu.tsx Outdated Show resolved Hide resolved
@jhnguyen521
Copy link
Contributor Author

jhnguyen521 commented Mar 13, 2024

For many components which are not from cmdk the applied changes do not work and cause the disable state to not work anymore. Means the fix: data-[disabled='true'] does not work for all components.

Good catch, thanks! I'll probably just revert changes to all other components since I'd probably have to make a custom class in css to get this to work for all while still being in the classNames. Should make for a simpler PR as well.

@benjamin-guibert
Copy link

Thank you for the fix!

@digitaltim-de
Copy link

Same Issue on the Example Filter Function, that dont works anymore too.

@nopitown
Copy link

Thanks for the fix!

@jhnguyen521
Copy link
Contributor Author

jhnguyen521 commented Jun 2, 2024

@jhnguyen521 Any chance you can fix the conflicts here please? Let's merge.

@shadcn Conflicts are fixed :)

ivan2214

This comment was marked as off-topic.

Copy link

vercel bot commented Jun 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ui ✅ Ready (Inspect) Visit Preview Aug 5, 2024 11:23am

@@ -117,7 +117,7 @@ const CommandItem = React.forwardRef<
<CommandPrimitive.Item
ref={ref}
className={cn(
"relative flex cursor-default select-none items-center rounded-sm px-2 py-1.5 text-sm outline-none data-[disabled=true]:pointer-events-none data-[selected=true]:bg-accent data-[selected=true]:text-accent-foreground data-[disabled=true]:opacity-50",
"relative flex cursor-default select-none items-center rounded-sm px-2 py-1.5 text-sm outline-none aria-[selected='true']:bg-accent aria-[selected='true']:text-accent-foreground data-[disabled='true']:pointer-events-none data-[disabled='true']:opacity-50",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use the new data-selected attribute instead if aria? Thank you.

…yen521/ui into fix-handling-data-disabled

# Conflicts:
#	apps/www/app/(app)/examples/playground/components/model-selector.tsx
@kotAPI
Copy link

kotAPI commented Jul 1, 2024

Any updates on this?

@saver711
Copy link

WE NEED THIS !!!

@shadcn
Copy link
Collaborator

shadcn commented Aug 5, 2024

@kevinmitch14 I merged #2626. Anything else we're missing? \cc @jhnguyen521

# Conflicts:
#	apps/www/app/(app)/examples/forms/account/account-form.tsx
#	apps/www/app/(app)/examples/playground/components/model-selector.tsx
#	apps/www/content/docs/components/combobox.mdx
#	apps/www/public/registry/styles/default/command.json
#	apps/www/public/registry/styles/new-york/command.json
#	apps/www/registry/default/ui/command.tsx
#	apps/www/registry/new-york/ui/command.tsx
@kevinmitch14
Copy link
Contributor

kevinmitch14 commented Aug 5, 2024

@shadcn it looks like some changes made in #2626 were reverted in #4181 by accident. (command.tsx, model-selector.tsx, cmdk package was reverted back to v0.2.0 files). I don't think this PR bumps it back to latest

@shadcn
Copy link
Collaborator

shadcn commented Aug 5, 2024

@kevinmitch14 Thanks. On it.

@shadcn shadcn merged commit 59a9310 into shadcn-ui:main Aug 5, 2024
6 checks passed
tangtai pushed a commit to tangtai/ui that referenced this pull request Aug 29, 2024
* fix: fix breaking changes for Command component that comes with cmdk 1.0.0

* chore: build registry

* chore: moving paths for some examples

* chore: moving paths for some examples

* chore: use data instead of aria

* fix: update command and pin cmdk

* fix: model selector

* fix: command for new york

---------

Co-authored-by: shadcn <m@shadcn.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Command/Combobox TypeError and Unclickable/Disabled items, cmdk breaking change