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

Replace deprecated NcMultiselect with recommended NcSelect #1471

Merged
merged 1 commit into from Mar 5, 2023

Conversation

susnux
Copy link
Collaborator

@susnux susnux commented Jan 30, 2023

NcMultiselect is deprecated the recommended alternative is NcSelect (mostly a drop-in replacement but few properties and slots changed or removed).

@susnux susnux added javascript Javascript related ticket 3. to review Waiting for reviews labels Jan 30, 2023
@Chartman123
Copy link
Collaborator

@susnux Looks good, but the styling does not fully fit the other components (border color and probably also the text color). I think that this should be fixed in the vue-lib.

Dark mode:
grafik

Light mode:
grafik

@susnux
Copy link
Collaborator Author

susnux commented Jan 31, 2023

@Chartman123 Do you emptied your cache? Exactly the opposite should be the case.
As NcMultiselect uses --color-border-dark while every other component, including NcSelect uses --color-border-maxcontrast

So it should now look like this:
darkmode, same borders but placeholder looks different
bright mode, same borders but placeholder looks different


But there are two differences:

  1. The NcSelect does not have primary color borders on hover and when selected, I already filed a PR to fix this: Adjust style of NcSelect to match other input elements nextcloud-libraries/nextcloud-vue#3701
  2. The placeholder looks differently because it uses --color-text-maxcontrast while "normal" inputs do not have a placeholder color set, this should be either fixed on the server side (global input::placeholder) or we could set this only for forms.

@susnux
Copy link
Collaborator Author

susnux commented Jan 31, 2023

Or maybe are you using NC 25.0.2? As the border was changed with this commit: nextcloud/server@20da75f on master (I am testing with current server master) which was backported to 25.0.3

Copy link
Member

@jotoeri jotoeri left a comment

Choose a reason for hiding this comment

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

Two small things here, apart from that, the theming does also not work properly....

(Indeed, quite specific, since System theme is dark, but NC Theme is set to light here...)
grafik

src/FormsSettings.vue Outdated Show resolved Hide resolved
src/components/Questions/QuestionDropdown.vue Outdated Show resolved Hide resolved
@Chartman123
Copy link
Collaborator

(Indeed, quite specific, since System theme is dark, but NC Theme is set to light here...)\n

That's how it looked for me yesterday at first. I then ran another npm run dev and then it changed to the way on my Screenshots... So probably really some caching problems here.

@susnux i will check the version later...

@susnux
Copy link
Collaborator Author

susnux commented Jan 31, 2023

(Indeed, quite specific, since System theme is dark, but NC Theme is set to light here...) 🖼️

This reminds me of nextcloud/text#3369

EDIT: 🥳 I finally found the origin of this issue, the theming variables are not set on the HTML tag but on the body tag. Fix: nextcloud/server#36462

@Chartman123
Copy link
Collaborator

@susnux: So should we merge this before the server fix?

@susnux
Copy link
Collaborator Author

susnux commented Feb 1, 2023

@susnux: So should we merge this before the server fix?

There is some discussion whether this is a server bug or should be fixed in the apps / libs. So I will file a PR against @nextcloud/vue so we get this fixed with the next release of it. I think we should wait for this to be fixed, so if we release before that, users might be annoyed by this bug.

@Chartman123
Copy link
Collaborator

@susnux The border color is now correct, but the text color still differs...

grafik

grafik

@Chartman123
Copy link
Collaborator

@susnux with the latest update of nc-vue the placeholder text is no longer shown

@susnux
Copy link
Collaborator Author

susnux commented Feb 28, 2023

with the latest update of nc-vue the placeholder text is no longer shown

A regression if the select is configured to be not searchable: nextcloud-libraries/nextcloud-vue#3833
(Probably fixed with 7.8.0)

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@susnux
Copy link
Collaborator Author

susnux commented Mar 5, 2023

Rebased onto main.
@Chartman123 as main is now using version 7.8.0 this issue is fixed.

@susnux susnux added this to the 3.2 milestone Mar 5, 2023
@jotoeri jotoeri modified the milestones: 3.2, 3.3 Mar 5, 2023
Copy link
Collaborator

@Chartman123 Chartman123 left a comment

Choose a reason for hiding this comment

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

@susnux the text color is still different to the other fields, but I think that we can merge this now :)

@Chartman123
Copy link
Collaborator

@susnux the text color is still different to the other fields, but I think that we can merge this now :)

@Chartman123 Chartman123 merged commit 0c864c5 into main Mar 5, 2023
@Chartman123 Chartman123 deleted the fix/use-ncselect branch March 5, 2023 22:18
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 javascript Javascript related ticket
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants