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.

Sign up for GitHub

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

Jump to bottom

[86byz6xq0][tooltip] added properties for paint arrow #1503

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

[86byz6xq0][tooltip] added properties for paint arrow #1503

wants to merge 9 commits into from
+91 −4

Conversation

ilyabrower
Copy link
Contributor

@ilyabrower ilyabrower commented Jul 9, 2024 •
edited
Loading

Motivation and Context

I've added two props for Tooltip.Popper to determine the color of the arrow.

How has this been tested?

Manually and I've added screenshot test

Screenshots (if appropriate):

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).
  • Nice improve.

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly or it's not required.
  • Unit tests are not broken.
  • I have added changelog note to corresponding CHANGELOG.md file with planned publish date.
  • I have added new unit tests on added of fixed functionality.

@ilyabrower ilyabrower self-assigned this Jul 9, 2024
@ilyabrower ilyabrower force-pushed the feature/tooltip-arrow-color branch from 38ac5e5 to 2a76118 Compare July 9, 2024 15:47
@ilyabrower ilyabrower requested review from msereniti and j-mnizhek and removed request for msereniti July 10, 2024 08:23
@ilyabrower ilyabrower marked this pull request as ready for review July 10, 2024 08:23
j-mnizhek
j-mnizhek requested changes Jul 10, 2024
View reviewed changes
Copy link
Contributor

@j-mnizhek j-mnizhek left a comment

Choose a reason for hiding this comment

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

  • Asked two questions in the inner chat.
  • Updated example a little bit ✨ Waiting for possibility to pass tokens to the new properties. Will approve as soon as it will work 😎

j-mnizhek
j-mnizhek approved these changes Jul 10, 2024
View reviewed changes
Copy link
Contributor

@j-mnizhek j-mnizhek left a comment

Choose a reason for hiding this comment

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

Updated example for colored Tooltip. Everything other looks good!

msereniti
msereniti reviewed Jul 12, 2024
View reviewed changes
semcore/tooltip/src/style/tooltip.shadow.css Outdated
@@ -72,6 +72,14 @@ SArrow[theme='warning'] {
}
}

SArrow[bgColor][shadowColor] {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it will be good to set them only when both are provided while it's two optional properties. Maybe we should provide it in one prop as object?

Copy link
Contributor Author

@ilyabrower ilyabrower Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

shadowColor has a fallback to bgColor, so, here it will always be defined if bgColor is defined.
I've removed shadowColor from condition here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j-mnizhek j-mnizhek j-mnizhek approved these changes

@msereniti msereniti Awaiting requested review from msereniti

Assignees

@ilyabrower ilyabrower

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ilyabrower @msereniti @j-mnizhek