Skip to content

feat:#243 add features component #265

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

Merged
merged 11 commits into from
Jan 31, 2025
Merged

Conversation

ss-dimasm
Copy link
Contributor

@ss-dimasm ss-dimasm commented Dec 26, 2024

Pull request checklist

Detail as per issue below (required):

part of #243

Context

Initialize a new component as part of elements v5 'Features

Design:

image

Preview (with Tooltip support):

2025-01-17_16-31-30.mp4

Copy link

codacy-production bot commented Dec 26, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for 09632f01 83.33%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (09632f0) Report Missing Report Missing Report Missing
Head commit (101f336) 4760 4242 89.12%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#265) 24 20 83.33%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Footnotes

  1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

@ss-dimasm ss-dimasm marked this pull request as ready for review December 26, 2024 10:46
@ss-dimasm ss-dimasm marked this pull request as draft January 17, 2025 08:26
@ss-dimasm ss-dimasm marked this pull request as ready for review January 17, 2025 09:29
@ksolanki7
Copy link
Contributor

@ss-dimasm The issue you discussed with me is maybe the race condition on the visibility of the Tooltip v/s the placement of the tooltip on the canvas. The solution to that can be adding data attribute to tooltip by updating the useTooltip hook -> getTooltipProps i.e. data-visible to boolean isVisible state and then managing the animation using CSS as below.

export const ElTooltip = styled.div`
  @keyframes fadeIn {
    from {
      opacity: 0;
    }
    to {
      opacity: 1;
    }
  }

  @keyframes fadeOut {
    from {
      opacity: 1;
    }
    to {
      opacity: 0;
    }
  }

  ... (rest of CSS)

  animation-duration: 0.2s;
  animation-fill-mode: both;
  animation-timing-function: ease-in-out;
  opacity: 0;

  &[data-visible='true'] {
    animation-name: fadeIn;
  }
  &[data-visible='false'] {
    animation-name: fadeOut;
  }

@ss-dimasm
Copy link
Contributor Author

Hey @ksolanki7 while I agree it's caused by the race condition that appear between javascript logic and the canvas, and we would fix it with CSS solution

I suppose it would be more handy if it have their own isolated ticket; hence it's not only appearing in the Features and I believe as far as it not changing the API it would not cause a major change, what do you say?

@ksolanki7
Copy link
Contributor

@ss-dimasm Yes, definately. This requires changes in the Tooltip itself i.e. at the component level and not at the consumer end.

Copy link
Contributor

@kurtdoherty kurtdoherty 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, just the one question which may or may not be a blocker.

Copy link
Contributor

@kurtdoherty kurtdoherty left a comment

Choose a reason for hiding this comment

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

Thanks @ss-dimasm. Just the one todo off the back of your answer.

@ss-dimasm ss-dimasm merged commit c42951a into main Jan 31, 2025
6 checks passed
@ss-dimasm ss-dimasm deleted the feat/#243-add-feature-component branch January 31, 2025 07:58
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.

3 participants