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

NcSelect: make same height and color as NcTextField #5829

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

GVodyanov
Copy link
Contributor

@GVodyanov GVodyanov commented Jul 18, 2024

☑️ Resolves

🖼️ Screenshots

🏚️ Before 🏡 After
image image

@GVodyanov GVodyanov self-assigned this Jul 19, 2024
@GVodyanov GVodyanov added the 3. to review Waiting for reviews label Jul 19, 2024
@GVodyanov
Copy link
Contributor Author

Known issue we decided to procrastinate, on Firefox it looks like this:

image

(there are no opacity or box-shadows set)

@GVodyanov GVodyanov marked this pull request as ready for review July 19, 2024 11:07
@GVodyanov GVodyanov requested a review from ChristophWurst July 19, 2024 11:09
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.

Hi @GVodyanov, the height of these elements needs to be --default-clickable-area, the input fields will also shrink a bit soon

@GVodyanov
Copy link
Contributor Author

GVodyanov commented Jul 19, 2024

Hi @GVodyanov, the height of these elements needs to be --default-clickable-area, the input fields will also shrink a bit soon

That's what it already should be set to actually @marcoambrosini

@GVodyanov GVodyanov requested a review from marcoambrosini July 19, 2024 14:01
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

The color changes should only be applied to the placeholder, not the input element as the input text must stay main text color.

src/components/NcInputField/NcInputField.vue Outdated Show resolved Hide resolved
src/components/NcSelect/NcSelect.vue Outdated Show resolved Hide resolved
src/components/NcSelect/NcSelect.vue Outdated Show resolved Hide resolved
@GVodyanov
Copy link
Contributor Author

I made the height of everything --default-clickable-area all the way through, does it seem ok?

@GVodyanov GVodyanov requested a review from susnux July 22, 2024 13:26
@GVodyanov GVodyanov force-pushed the style/slim-down-nc-select branch from 3f659f2 to c7d879e Compare July 24, 2024 07:47
@GVodyanov GVodyanov force-pushed the style/slim-down-nc-select branch from c7d879e to 02a20a4 Compare July 24, 2024 14:05
@GVodyanov GVodyanov merged commit b2d8c9e into master Jul 24, 2024
19 checks passed
@GVodyanov GVodyanov deleted the style/slim-down-nc-select branch July 24, 2024 14:08
@susnux susnux added bug Something isn't working design Design, UX, interface and interaction design labels Jul 24, 2024
@susnux
Copy link
Contributor

susnux commented Jul 24, 2024

Please remember to add correct labels:

  1. State of the PR (e.g. 3. to review)
  2. But more important the type of the PR: bug, enhancement or refactor this is needed for changelog generation.

Also always remember to backport to next (there is a TODO section in the PR template for this :) ).

@susnux
Copy link
Contributor

susnux commented Jul 24, 2024

/backport to next

@@ -1263,12 +1269,13 @@ body {

.vs__selected-options {
// If search is hidden, ensure that the height of the search is the same
min-height: 40px; // 36px search height + 4px search margin
min-height: var(--default-clickable-area);
Copy link
Contributor

@Antreesy Antreesy Jul 29, 2024

Choose a reason for hiding this comment

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

Selector specificity either isn't big enough or scoped enough here (see leaked styles from server and viewer):
image

Not a part of this PR, of course, but something to consider

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 #5883

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 bug Something isn't working design Design, UX, interface and interaction design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent font and height of event resource inputs
4 participants