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

Change styles for focused Select #4728

Conversation

JuliaKirschenheuter
Copy link
Contributor

☑️ Resolves

🖼️ Screenshots

🏚️ Before

Peek 2023-10-31 14-05

🏡 After

Peek 2023-10-31 14-04

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@raimund-schluessler
Copy link
Contributor

This introduces a (thin) line between the input and the options, which was not there before:
grafik

And for the NcSelectTags component, the border/outline seems to be offset on the right:
grafik

Copy link
Contributor

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

Nice, a couple more things:

There seems to be a border misalignment in one of your screenshots:

Screenshot 2023-11-01 at 12 54 19

Can we remove all box shadows from the select options container?
Screenshot 2023-11-01 at 13 07 23

Thanks :)

@JuliaKirschenheuter JuliaKirschenheuter marked this pull request as draft November 2, 2023 15:37
@JuliaKirschenheuter
Copy link
Contributor Author

And for the NcSelectTags component, the border/outline seems to be offset on the right:

Dear @raimund-schluessler

unfortunately i can't reproduce it:
Screenshot from 2023-11-02 16-40-46

could you please try again?

@marcoambrosini could you please help me a bit to find box-shadow styles?

@raimund-schluessler
Copy link
Contributor

And for the NcSelectTags component, the border/outline seems to be offset on the right:

Dear @raimund-schluessler

unfortunately i can't reproduce it

could you please try again?

It seems to be fixed. I don't see it anymore either. However, the border at the bottom is still blue when the select is opened to the top. See this screenshot from the docs:
grafik

@JuliaKirschenheuter
Copy link
Contributor Author

@raimund-schluessler styles of my local styleguide instance are broken ;( I guess i've fixed an issue, could you please re-review? Thanks a lot!

@JuliaKirschenheuter JuliaKirschenheuter marked this pull request as ready for review November 3, 2023 10:29
@raimund-schluessler
Copy link
Contributor

@raimund-schluessler styles of my local styleguide instance are broken ;( I guess i've fixed an issue, could you please re-review? Thanks a lot!

The border is fixed now. 👍

@raimund-schluessler
Copy link
Contributor

@JuliaKirschenheuter Sorry, I just saw there is another box-shadow when the dropdown opens to the top. This line would also need to go:

box-shadow: 0px -1px 1px 0px var(--color-box-shadow) !important;

And could you please rebase your branch so that it includes all latest changes from master? I think there were some recent adjustments of NcSelect and it would be good to test them with your changes before merging.

Signed-off-by: julia.kirschenheuter <[email protected]>
@@ -1182,8 +1184,10 @@ body {
}

.vs__dropdown-menu {
border-color: var(--color-primary-element) !important;
border-color: var(--color-main-text) !important;
outline: 2px solid var(--color-main-background);
Copy link
Contributor

Choose a reason for hiding this comment

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

This outline adds a not so nice gap between the select toggle and the dropdown-menu. But I am not sure we can fix this.
grafik

More obvious with adjusted outline color
grafik

@susnux Any idea here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, without this outline it looks odd too like from your video:

After.mp4

I think it is better to keep outline. And all other improvements we can do after a11y certification

Copy link
Contributor

Choose a reason for hiding this comment

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

well, without this outline it looks odd too like from your video:

That's unfortunately true. One could fix it with an additional container around the dropdown and some CSS including a margin-top: -2px;, but that would require adjusting @nextcloud/vue-select which is out out scope here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the vertical position of the drop down needs to be changed. But not sure this is possible without changing the vue-select

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the vertical position of the drop down needs to be changed. But not sure this is possible without changing the vue-select

I tested this (manually). It won't help, since both elements, the trigger and the dropdown, have an outline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

=(
could we merge this pr and improve left things in next future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i will merge it now, got enough approves

@JuliaKirschenheuter JuliaKirschenheuter merged commit 2069548 into master Nov 6, 2023
15 checks passed
@JuliaKirschenheuter JuliaKirschenheuter deleted the fix/41217-Contrast_of_focused_select_is_not_enough branch November 6, 2023 08:35
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BITV]: Contrast of focused select is not enough
4 participants