Skip to content

Conversation

marcellamaki
Copy link
Member

@marcellamaki marcellamaki commented Sep 18, 2025

Summary

There was existing code to scroll to the storage form when it opened, but it wasn't working. This makes a few small tweaks and now it properly scrolls

To fix the checkbox clickability issue, both item-text, item-value props have been removed from the MultiSelect component as they are a source of confusion both in implementation and usage. Also, the the standard contract for the items prop is below as expected by vuetify's v-select;

[
    {
        "text": "<string to display>"
        "value": "<actual value of the selected item>"
    }
]

This pr affects the search filters in the import from other channel in addition to the request form

References

Fixes #5389

Screen.Recording.2025-09-18.at.9.06.28.AM.mov

Reviewer guidance

Go to Settings > Storage and open the storage form. It should now autoscroll so that the form start is at the top of the page. Previously, the form opened below and it was hard to tell that anything happened.

Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Thanks @marcellamaki Code changes LGTM. It however appears that the other issue of the inability to click the checkboxes in the dropdown hasn't been resolved. Are we resolving it with this pr? If not, we should be good to go!
image

@marcellamaki
Copy link
Member Author

🤦‍♀️ I just completely forgot about that bit from the issue. I can add that in, thanks @akolson -- sorry I totally missed that

Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

Requesting a small change to match what seems to be the prior intent of the storage form multi select

const options = sortBy(this.channels, c => c.name.toLowerCase()).filter(c => !c.public);
return options.map(option => ({
text: option.name,
value: option.id,
Copy link
Member

Choose a reason for hiding this comment

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

It seems the MultiSelect component didn't accept a function for customizing itemValue, but the implementation here expected that it would. So the appropriate change would be to format the value to match what the deleted method would return.

} else if (typeof this.itemText === 'function') {
return this.itemText(item);
}
return item.text || item;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any usages of this fallback behavior || item, so the template changes seem good.

const options = sortBy(this.channels, c => c.name.toLowerCase()).filter(c => !c.public);
return options.map(option => ({
text: option.name,
value: this.channelName(option),
Copy link
Member

Choose a reason for hiding this comment

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

@bjester we should be good now I think?

@akolson akolson requested a review from bjester September 19, 2025 15:46
Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

Tested and LGTM

@akolson akolson merged commit 8a20813 into learningequality:hotfixes Sep 19, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants