Skip to content

Add color prop to VaDivider component and bind it to CSS border color#3154

Merged
rustem-nasyrov merged 5 commits intoepicmaxco:developfrom
Yasir761:change-color-prop
Mar 24, 2023
Merged

Add color prop to VaDivider component and bind it to CSS border color#3154
rustem-nasyrov merged 5 commits intoepicmaxco:developfrom
Yasir761:change-color-prop

Conversation

@Yasir761
Copy link
Copy Markdown
Contributor

@Yasir761 Yasir761 commented Mar 13, 2023

Close #3153

…ers to specify the color of the divider. The color prop is then bound to the CSS border-color property using Vue's v-bind in CSS feature. This enables users to dynamically change the divider color using the color prop.

Description

Markup:

Details
// Your code

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)

…ers to specify the color of the divider. The color prop is then bound to the CSS border-color property using Vue's v-bind in CSS feature. This enables users to dynamically change the divider color using the color prop.
@Yasir761
Copy link
Copy Markdown
Contributor Author

Please let me know, if there is chance to make a change.

Copy link
Copy Markdown
Contributor

@rustem-nasyrov rustem-nasyrov left a comment

Choose a reason for hiding this comment

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

Hello, @Yasir761! Thank you for your pull request. Here is a few suggestions.

  • Redundant color prop.
  • Let's create a common CSS variable for the border color and use it for horizontal and vertical views. Tip: you can create CSS variable in the _variables.scss file at the va-divider directory.

…g the border color in the va-divider component. This commit refactors the code to use a common CSS variable --va-divider-color for defining the border color, which can be used for both horizontal and vertical views. The CSS variable has been added to the :root and :host selectors in the _variables.scss file in the va-divider directory. This results in cleaner and more maintainable code
@m0ksem
Copy link
Copy Markdown
Member

m0ksem commented Mar 13, 2023

  • Redundant color prop.

    • Let's create a common CSS variable for the border color and use it for horizontal and vertical views. Tip: you can create CSS variable in the _variables.scss file at the va-divider directory.

@rustem-nasyrov, I would prefer to have color prop, the same way we have with any other component (like button, input, swtich etc). Kind of intuitive to have this prop despite it can be done easily with css variables. It is more about DX.

Compare this:

<va-divier :color="isDark ? 'text-light' : 'text-dark'" />
<va-divider :style="`--va-divider-color: ${getColor(isDark ? 'text-light' : 'text-dark')}`" />

m0ksem added 2 commits March 20, 2023 14:57
…pecifying the border color in the va-divider component. This commit refactors the code to use a common CSS variable --va-divider-color for defining the border color, which can be used for both horizontal and vertical views. The CSS variable has been added to the :root and :host selectors in the _variables.scss file in the va-divider directory. This results in cleaner and more maintainable code"

This reverts commit b4cd213.
@m0ksem m0ksem requested a review from rustem-nasyrov March 20, 2023 13:16
Copy link
Copy Markdown
Contributor

@rustem-nasyrov rustem-nasyrov left a comment

Choose a reason for hiding this comment

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

Keep it up! Just need to do a little fix and issue will be solved! :)

@rustem-nasyrov rustem-nasyrov merged commit ab4dd11 into epicmaxco:develop Mar 24, 2023
@Yasir761 Yasir761 deleted the change-color-prop branch March 24, 2023 09:48
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.

Divider is missing color prop

3 participants