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

Inconsistent behavior for VaTimeInput right icon #3922 #3933

Merged
merged 6 commits into from
Oct 12, 2023

Conversation

Udaay
Copy link
Contributor

@Udaay Udaay commented Oct 1, 2023

close #3922

Description

Changed the onClick handler of va-input-wrapper

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement/refactoring (non-breaking change that doesn't add any feature but make things better)

@Udaay Udaay changed the title fix: replaced toggleDropdown with showDropdown Inconsistent behavior for VaTimeInput right icon #3922 Oct 1, 2023
@asvae
Copy link
Member

asvae commented Oct 2, 2023

Hey. I was mistaken to reference naive ui.

Instead I should've mentioned vuestic-ui select. https://ui-storybook-develop.vuestic.dev/?path=/story/vaselect--old-demos

Naive does not close elements on click. But vuestic does. So we need to have toggle behavior for both icon and wrapper.

pointer-events: none on icon could do the trick :)

@Udaay
Copy link
Contributor Author

Udaay commented Oct 2, 2023

Yeah that will do the trick, I will make changes.

@m0ksem m0ksem self-requested a review October 8, 2023 23:50
Copy link
Collaborator

@m0ksem m0ksem 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 for me, let's deal with CSS variable and merge it.

@@ -407,4 +401,8 @@ export default defineComponent({

<style lang="scss">
@import "variables";

.va-time-input__side-button {
pointer-events: var(--va-time-input-side-button-pointer-events);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey, @Udaay. We use CSS variables, so it is easier for user to change component look. Do you think there is a case when user need to change pointer-events behavior?

The perfect examples of using css variables to configure border-radius, border-width, paddings, margins, etc.

What I don't like is to have css variables, that for 99% will not be changed. Like display (which is set to flex and whole component breaks if display is changed), position (which must be relative otherwise everything breaks).

We actually do not follow this rule completely, we have a lot of old stuff which was converted to css variables by a script. And I hope in future we will have less meaningless variables.

If you find pointer-events css variable helpful here, let's have it, but if not please remove the css variable. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes , I got your point and whole idea behind the css variables so that user can configure some specific styles.
Will remove this variable and add directly.

@@ -1,4 +1,5 @@
:root,
:host {
--va-time-input-min-width: var(--va-form-element-min-width);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm also not sure if --va-time-input-min-width is used somewhere, so this can be removed too likely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me check, I will remove it if it doesn't required.

Copy link
Collaborator

@m0ksem m0ksem left a comment

Choose a reason for hiding this comment

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

Thanks!

@m0ksem m0ksem merged commit 080b053 into epicmaxco:develop Oct 12, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent behavior for VaTimeInput right icon
3 participants