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: Use simple input field instead of multiselect for plain URLs #787

Merged
merged 4 commits into from
Aug 30, 2024

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jan 19, 2024

When no link provider (except URL) is enabled a multiselect field might be confusing to users, so this PR switches over to a simple input field in that case. As there are no suggestions then a multiselect does not make sense.

Styling of the multiselect is still a bit odd, but will check that separately as it seems to come from the component library.

Kapture.2024-08-22.at.15.54.06.webm

@juliusknorr juliusknorr added enhancement New feature or request design Related to the design papercut Small issues that doesn't break the ux/ui 3. to review Waiting for reviews labels Jan 19, 2024
@juliusknorr juliusknorr added this to the v0.7.0 milestone Jan 19, 2024
Copy link
Contributor

@enjeck enjeck left a comment

Choose a reason for hiding this comment

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

Looks good except for the failing Cypress tests. Pushed a commit for this

@enjeck
Copy link
Contributor

enjeck commented Jan 21, 2024

Weird that the test still fails on some stable branches 🤔

@datenangebot
Copy link
Collaborator

How to reproduce this? I removed the provider stuff directly on db side like [{"columnId":1001,"value":"http://phpmyadmin.local"}] but there is no change on the front end?!

Thoughts:

  • Is it possible to look for other resources if it is a plain url presented as a input?
  • Wouldn't it be better if we change plain URLs to link providder URL and fetch the title from the url as title or subline?

@juliusknorr
Copy link
Member Author

Sorry I should have described this a bit more. The goal of this PR is purely to make the input more user friendly if only URL is enabled as a provider for a column, as in this case the multiselect does not give any benefit as there is only one option to select (which is inserting the plain URL).

I agree we should still fetch the title and store/render it for additional context. Will look into that.

@juliusknorr juliusknorr removed this from the v0.7.0 milestone Feb 15, 2024
@enjeck
Copy link
Contributor

enjeck commented May 6, 2024

@juliushaertl Do we still need this?

@juliusknorr juliusknorr self-assigned this May 10, 2024
@juliusknorr
Copy link
Member Author

Yes, I think this would still be a nice improvement.

Will try to wrap it up

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member Author

Ok that has been a while 🙈

Finally pushed some more adjustments here all summarized with a screen recording in the first post.

Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
@juliusknorr juliusknorr requested review from enjeck and luka-nextcloud and removed request for datenangebot August 27, 2024 13:49
Copy link
Contributor

@enjeck enjeck left a comment

Choose a reason for hiding this comment

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

Shouldn't the multiselect be bigger so that everything fits in? Seems weird to have to scroll through such a narrow field to see the full content

Screenshot from 2024-08-30 04-58-40

…elect

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member Author

I failed to properly adapt the NcSelect to a dynamic height for the selected-option slot, so I decided for now to just stick to a standard height and remove the link type when showing the current selection. Should be not relevant anyways as we have a more detailed link preview:

Screenshot 2024-08-30 at 08 28 14 Screenshot 2024-08-30 at 08 28 08

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Copy link
Contributor

@enjeck enjeck left a comment

Choose a reason for hiding this comment

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

I like it 😎 👍

@juliusknorr juliusknorr merged commit 0fee7b8 into main Aug 30, 2024
47 checks passed
@juliusknorr juliusknorr deleted the enh/simple-link-input branch August 30, 2024 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Related to the design enhancement New feature or request papercut Small issues that doesn't break the ux/ui
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Unable to get back previously set (github) link
3 participants