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(tailwind): first version of RefInput to show REF as radiogroup and REF_ARRAY as checkboxgroup #4585

Merged
merged 20 commits into from
Jan 30, 2025

Conversation

mswertz
Copy link
Member

@mswertz mswertz commented Dec 30, 2024

What are the main changes you did

  • created first InputRef to enable Form to work for REF and REF_ARRAY
  • I propose to merge this first and then refactor to enable user to chose for select, multiselect, checkbox or radio, see todo

How to test

  • see the test pages -> either 'Ref' or 'Form'
  • see e2e test

@mswertz mswertz marked this pull request as draft December 30, 2024 16:14
@mswertz mswertz changed the title feat: first version of RefArrayInput feat(tailwind): first version of RefArrayInput Dec 30, 2024
@mswertz mswertz marked this pull request as ready for review December 31, 2024 22:29
@mswertz mswertz changed the title feat(tailwind): first version of RefArrayInput feat(tailwind): first version of RefInput to show REF as radiogroup and REF_ARRAY as checkboxgroup Jan 2, 2025
@mswertz mswertz mentioned this pull request Jan 10, 2025
8 tasks
Copy link
Contributor

@davidruvolo51 davidruvolo51 left a comment

Choose a reason for hiding this comment

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

I'm curious to see how this component overlaps/interacts with the listbox component. I think the data flows are similar. I have a few suggestions, but some of my comments might be beyond this PR and be specific to the TW implementation overall.

Copy link
Contributor

@chinook25 chinook25 left a comment

Choose a reason for hiding this comment

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

While clicking around on the preview server I noticed two things for the ref-input:

  • The clear all button causes the component to flash while updating.
  • Doing several interactions with it in quick succession causes performance issues. It causes a spike in CPU usages and even makes my mouse lagg.

@mswertz
Copy link
Member Author

mswertz commented Jan 29, 2025

  • The clear all button causes the component to flash while updating.

Do you know how to prevent? It is because the options are re-rendered.

@mswertz
Copy link
Member Author

mswertz commented Jan 29, 2025

  • Doing several interactions with it in quick succession causes performance issues. It causes a spike in CPU usages and even makes my mouse lagg.

I suppose this is because of that options are retrieved from server and that involves an await? Do you have recommendations how to counter this?

@mswertz mswertz requested a review from davidruvolo51 January 29, 2025 22:40
@@ -44,6 +44,12 @@ const SIZE_MAPPING = {
medium: "h-14 px-7.5 text-heading-xl gap-4",
large: "h-18 px-8.75 text-heading-xl gap-5",
};
const ICON_SIZE_MAPPING = {
tiny: 12,
Copy link
Member Author

Choose a reason for hiding this comment

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

there is no way tailwind takes care of this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think it's possible to add them to css/tailwind files. We would also need to refactor this component as there a lot of the javascript could be replaced by simpler tw classes. It would be good to do it in the next PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Leave it for now and fix in another PR

@@ -0,0 +1,28 @@
<script setup lang="ts">
Copy link
Member Author

Choose a reason for hiding this comment

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

how to call this component? And I notice the css doesn't work on all themes properly. Need help there.

Copy link
Contributor

@davidruvolo51 davidruvolo51 left a comment

Choose a reason for hiding this comment

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

This is really great. Some of my comments might be better as follow up PRs, but there are some missing tailwind classes (e.g., search input border, text colours, etc.).

@@ -44,6 +44,12 @@ const SIZE_MAPPING = {
medium: "h-14 px-7.5 text-heading-xl gap-4",
large: "h-18 px-8.75 text-heading-xl gap-5",
};
const ICON_SIZE_MAPPING = {
tiny: 12,
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think it's possible to add them to css/tailwind files. We would also need to refactor this component as there a lot of the javascript could be replaced by simpler tw classes. It would be good to do it in the next PR.

</Button>
</div>
<div class="flex flex-wrap gap-2 mb-2">
<ButtonText
Copy link
Contributor

Choose a reason for hiding this comment

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

The text color that is used in the ButtonText component is yellow. This should probably be text-blue-500. We should probably assign it to a tw class so that we can use it everywhere, e.g., *-button-text

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed. added the class text-color-button-text

<InputLabel :for="`search-for-${id}`" class="sr-only">
search in {{ columnName }}
</InputLabel>
<InputSearch
Copy link
Contributor

Choose a reason for hiding this comment

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

In the pet store form example, the search does not display and there is some content shifting. Maybe the v-if logic needs to be adjusted?

See the simple form story and the column Category

class="flex flex-wrap gap-2 mb-2"
v-if="isArray ? selection.length : selection"
>
<Button
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a lot of content shifting with the filters. I'm not quite sure how to solve this. Maybe we need to wrap this in an expandable section?

/>
</template>
<template v-if="!hasNoResults">
<InputCheckboxGroup
Copy link
Contributor

Choose a reason for hiding this comment

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

Some tw classes are missing here or are overwritten somewhere. The checkbox border does not appear.

In the form story, select the complex example and scroll to the "Resources" field in the population chapter.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is also some strange scroll behaviour that occurs after clicking the checkbox. In the "resources" field, if you click on "testCohort3", the view is shifted down the page. This is probably due to some content shift and focus is placed somewhere else. It's probably an issue with the form component or maybe the "values" section in the right sidebar.

Copy link
Contributor

@davidruvolo51 davidruvolo51 Jan 30, 2025

Choose a reason for hiding this comment

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

This only happens when the sidebar is open. 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

To investigate in a further PR

@deselect="deselect"
:inverted="inverted"
/>
<ButtonText
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we limit the number of visible options or put this into a scrollable area? In the complex form example, I continuously click the "load 10 more" button. The number of visible options becomes quite long and there is content shift. It might be better to place this in a scrollable area to prevent forms from growing in size. I might also make the search visible by default in this case.

type="search"
:value="modelValue"
@input="(event) => handleInput((event.target as HTMLInputElement).value)"
class="w-full pr-4 font-sans text-black bg-white outline-none rounded-search-input h-10 ring-red-500 pl-10 shadow-search-input focus:shadow-search-input hover:shadow-search-input"
Copy link
Contributor

Choose a reason for hiding this comment

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

The border is missing in the default theme. I think it's caused by the inverted property. Perhaps we can remove this.

Copy link
Contributor

@davidruvolo51 davidruvolo51 Jan 30, 2025

Choose a reason for hiding this comment

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

Hmm, this isn't as straightforward as I thought. This component seems to be styled for specific search pages or use cases. Changing the styles in the component would affect the instances where it is used. Not sure if we want to change it here. It tried to override the styles in the ref component with [&>div>input]:border-gray-600, but the inverted prop seems to block this. Either we create variants of the search input component or style these in their context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will fix in another PR

updateSearch(""); //reset
}

function loadMore() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This causes content shifting or flickering. I would expect the load more to retrieve n records after the current set. It should append them to the current list rather than rebuilding the list of options.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fix in another PR

Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above, the tw border classes is missing here or overwritten by the inverted prop.

@mswertz mswertz merged commit dcabeff into master Jan 30, 2025
7 checks passed
@mswertz mswertz deleted the feat/tailwind_refinput branch January 30, 2025 19:13
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.

None yet

3 participants