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 criteria form #5827

Merged
merged 3 commits into from
Oct 4, 2024
Merged

Fix criteria form #5827

merged 3 commits into from
Oct 4, 2024

Conversation

lovincyrus
Copy link
Contributor

@lovincyrus lovincyrus commented Oct 3, 2024

@briangregoryholmes
Copy link
Contributor

briangregoryholmes commented Oct 4, 2024

The Notion doc is referring to a different element of the form. I'm also not able to reproduce the Notion screenshot to begin with. The default behavior of the Select component is to fully fit the label, so I'm not totally sure where this issue would have come from.

As for the last element, it's not really that you want width to be "null", but rather "auto" or some fixed value like "120px". I'd rather not accept null here.

That said, I do see some issues with the Select component not always respecting the width property (it fits rather than sets a fixed width). I can look into that.

A larger issue is that this form does not appear to function properly. The measure name dropdown for each criteria only ever has one option and the ids are in conflict, so the triggers open up menus in the wrong places (though they do work). That latter one may have only surfaced when moving to shadcn a few weeks ago.

@lovincyrus lovincyrus merged commit 0d55346 into main Oct 4, 2024
7 checks passed
@lovincyrus lovincyrus deleted the cyrus/visible-criteria-form branch October 4, 2024 21:49
k-anshul pushed a commit that referenced this pull request Oct 15, 2024
* offer null to width prop in input

* revert

* feedback
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.

2 participants