-
Notifications
You must be signed in to change notification settings - Fork 15
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
[CPT-2535] Add property to place accordion icon on the left #4522
[CPT-2535] Add property to place accordion icon on the left #4522
Conversation
🦋 Changeset detectedLatest commit: 9ea5407 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Greetings from FX team, @angelinastavniiciuc 👋 Thank you so much for contributing 🙇 We have got high priority ticket generated on our Kanban board so we will do our best to make your experience supreme! What's next? We will collaborate using this workflow. For you this practically means making sure DONE criteria is met and responding promptly to code review comments 😉 🙏 please, help us improve, rate your contributing experience after merge |
@toptal-anvil ping reviewers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@angelinastavniiciuc Can you please confirm that Design Team approved this change? Is there a Figma or https://toptal.enterprise.slack.com/archives/CCC3GP6CC discussion for this decision?
Can you please also check why visual changes affect existing stories, can it be avoided?
Hello @sashuk, we created this ticket because we had to work around it in the Staff portal / Call assistant to place the accordion arrow on the left. It worked by overriding the styles.
What do you think? |
Hello, @toptalwadiibasmi! Let's double check with Design Team here, as the designs that Picasso is supposed to follow do not have this case |
Hey @sashuk yep I asked design team for feedback about this change and they confirmed it, here is the thread https://toptal-core.slack.com/archives/C02AN1SSGK0/p1725363298979669 |
cbbb8e1
to
69dd83e
Compare
Thank you @angelinastavniiciuc for checking! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, the only visual addition is approved
@toptal-anvil ping reviewers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA approved ✅
3c0b05f
to
9ea5407
Compare
CPT-2535
Description
While working on Call Assistant modal there was a design where expand Accordion button is placed to the left, there was no possibility to add it via Picasso so we implemented workaround solution. In this PR we added this prop and provided an example for it.
How to test
Screenshots
Development checks
picasso-tailwind-merge
requires major update (check itsREADME.md
)props
in component with documentationexamples
for componentBreaking change
PR commands
List of available commands:
@toptal-bot run package:alpha-release
- Release alpha version@toptal-anvil ping reviewers
- Ping FX team for reviewPR Review Guidelines
When to approve? ✅
You are OK with merging this PR and
nit:
to your comment. (ex.nit: I'd rename this variable from makeCircle to getCircle
)When to request changes? ❌
You are not OK with merging this PR because
When to comment (neither ✅ nor ❌)
You want your comments to be addressed before merging this PR in cases like:
How to handle the comments?