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

modify feedback widget behaviour #8312

Merged
merged 7 commits into from
Nov 14, 2022

Conversation

malunem
Copy link
Contributor

@malunem malunem commented Oct 20, 2022

Description

  • Add isExpanded state to FeedbackWidget component
  • Set a 30sec timeout as a side effect to change isExpanded state to true and clear timeout on unmount
  • Use media query to apply style changes only on desktop devices
  • Apply 0.5 transition timing for width and border-radius
  • Reset isExpanded to false when isOpen is reset too
  • Import Text component from ChakraUI
  • Truncate text while expanding to show a smooth animation

Related Issue

#8298

@malunem malunem changed the title modify feedback widget behaviour #8298 modify feedback widget behaviour Oct 20, 2022
@gatsby-cloud
Copy link

gatsby-cloud bot commented Oct 20, 2022

✅ ethereum-org-website-dev deploy preview ready

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @marcellamalune. I think the basic animation is working but there are some problems when the expanding animation starts. Take a look at this to see it clearly:
https://user-images.githubusercontent.com/468158/198043249-d6e3e1f4-5c69-4c76-bb2f-c46ed4ea9608.mp4

src/components/FeedbackWidget.tsx Outdated Show resolved Hide resolved
@pettinarip
Copy link
Member

This is looking nice so far @marcellamalune. Wanted to check with @konopkja what he thinks about it? should the animation be faster? or is it ok as it is?

@konopkja
Copy link
Contributor

looks nice! agree, the animation could be twice as fast. other than that it looks good.

@malunem
Copy link
Contributor Author

malunem commented Oct 27, 2022

Thank you for your feedback @pettinarip @konopkja! I speeded up the animation.

@konopkja
Copy link
Contributor

  • Deploy preview

ok this is better. @pettinarip im good with this.

@malunem malunem force-pushed the feature/expand-feedback-widget branch from e9d2d85 to 7092831 Compare October 28, 2022 17:19
@corwintines
Copy link
Member

Screen Shot 2022-10-31 at 12 01 38 PM

Hey @marcellamalune!

Noticing this isn't working on mobile.

@malunem
Copy link
Contributor Author

malunem commented Oct 31, 2022

Screen Shot 2022-10-31 at 12 01 38 PM

Hey @marcellamalune!

Noticing this isn't working on mobile.

Sorry about that, I screwed up something with the latest changes without realizing. Now it's fixed.

@corwintines
Copy link
Member

Thanks for fixing @marcellamalune. Will get to review this week, but I did add the hacktoberfest-accepted label to get you credit for this still.

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

The overall result looks great right now @marcellamalune. My only concern is more about the implementation using ids, wondering if we could adjust a bit that.

Comment on lines 42 to 47
#feedback-wrapper {
width: ${({ isExpanded }) => (isExpanded ? "13.5rem" : "3rem")};
position: ${({ isExpanded }) => (isExpanded ? "absolute" : "flex")};
}
#expanded-prompt {
display: ${({ isExpanded }) => (isExpanded ? "flex" : "none")};
Copy link
Member

Choose a reason for hiding this comment

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

I don't love the idea of using ids to achieve this. Why can't we implement this logic using style props?

Can't we do this like

<Text ... width={{ base: "3rem", lg: isExpanded ? "13.5rem" : "3rem" }}>
  <Box
    as={Translation}
    id="feedback-card-prompt-page"
    display={{ base: "flex", lg: isExpanded ? "flex" : "none" }}
  />
</Text>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pettinarip thanks for the feedback and for the improvements you've made 🙂

@konopkja
Copy link
Contributor

@pettinarip @marcellamalune can we push this forward?

@pettinarip
Copy link
Member

@pettinarip @marcellamalune can we push this forward?

I think this is done. I'll do some small tweaks and get it merged.

Copy link
Member

@corwintines corwintines left a comment

Choose a reason for hiding this comment

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

Thanks @marcellamalune and @pettinarip!

@corwintines corwintines merged commit e75ebb2 into ethereum:dev Nov 14, 2022
@corwintines corwintines mentioned this pull request Nov 14, 2022
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.

None yet

4 participants