Skip to content

feat: Add css transitions #2511

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
Merged
JayaKrishnaNamburu merged 16 commits into from
Nov 1, 2023
Merged

feat: Add css transitions #2511

JayaKrishnaNamburu merged 16 commits into from
Nov 1, 2023

Conversation

@JayaKrishnaNamburu
Copy link
Contributor

@JayaKrishnaNamburu JayaKrishnaNamburu commented Oct 20, 2023 •
edited
Loading

Description

Adds support for css-transitions when users are in local state. In any other state than local, the CRUD operations on transitions are disabled.

@vercel
Copy link

vercel bot commented Oct 20, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
webstudio-builder ✅ Ready (Inspect) Visit Preview Oct 31, 2023 1:55pm

@JayaKrishnaNamburu JayaKrishnaNamburu changed the title Add css transitions WIP: Add css transitions Oct 20, 2023
@JayaKrishnaNamburu JayaKrishnaNamburu mentioned this pull request Oct 20, 2023
Closed
7 tasks
@JayaKrishnaNamburu JayaKrishnaNamburu changed the title WIP: Add css transitions feat: Add css transitions Oct 22, 2023
@JayaKrishnaNamburu
Copy link
Contributor Author

JayaKrishnaNamburu commented Oct 22, 2023

@taylornowotny @kof the transitions section is in usable state. The UX is very similar to box-shadow. So give it a try and see the changes needed.

@kof
Copy link
Member

kof commented Oct 22, 2023

@JayaKrishnaNamburu testing

@kof
Copy link
Member

kof commented Oct 22, 2023

  • I think it makes sense to have WF defaults when adding transition:
Screenshot 2023-10-22 at 10 11 09

@JayaKrishnaNamburu JayaKrishnaNamburu marked this pull request as ready for review October 23, 2023 09:27
@kof
Copy link
Member

kof commented Oct 23, 2023

Is the feature still behind a feature flag? because we can't merge if user will see it as-is right away

istarkov
istarkov reviewed Oct 23, 2023
View reviewed changes
istarkov
istarkov reviewed Oct 23, 2023
View reviewed changes
istarkov
istarkov reviewed Oct 23, 2023
View reviewed changes
@JayaKrishnaNamburu JayaKrishnaNamburu changed the title WIP: feat: Add css transitions feat: Add css transitions Oct 27, 2023
@kof kof requested review from istarkov and taylornowotny and removed request for TrySound October 27, 2023 21:12
taylornowotny
taylornowotny suggested changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@taylornowotny taylornowotny left a comment

Choose a reason for hiding this comment

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

Transitions panel UI not yet implemented

Remove button bug that I pointed out is still there

@JayaKrishnaNamburu
Copy link
Contributor Author

JayaKrishnaNamburu commented Oct 30, 2023 •
edited
Loading

@taylornowotny i just updated the upstream. Did't push any chaanges yet. Will let you know. And reverted all the changes regarding the UI panel. They will be coming in a different PR one after another.

@JayaKrishnaNamburu
Copy link
Contributor Author

JayaKrishnaNamburu commented Oct 30, 2023 •
edited
Loading

And @taylornowotny i just tested it. It's not bug, but more of a UX thing maybe ?
Because once the floating panel is closed. The focus is moved back to the element that is just being edited. So, the selector is kicking and marking them as visible.

https://github.com/webstudio-is/webstudio/blob/main/packages/design-system/src/components/css-value-list-item.tsx#L121

Screen.Recording.2023-10-30.at.21.44.47.mov

So, it's either change the list-item to look like highlighted. Or don't show the icons even if the focus persists. Your choice 👍

@taylornowotny
Copy link
Contributor

taylornowotny commented Oct 30, 2023 •
edited
Loading

@JayaKrishnaNamburu

After the menu closes, the item should not be focused.

Does that answer your question?

@kof Pinging you to double-check the intended focus behavior for the "List/List Item" component.

@JayaKrishnaNamburu JayaKrishnaNamburu mentioned this pull request Oct 30, 2023
Merged
@kof
Copy link
Member

kof commented Oct 31, 2023

@taylornowotny I just checked, its the same behavior in production, so this pr didn't change it, I would suggest always try on main branch to see if the change was implemented here.

What seems wrong about the current state is not the focus behavior itself, but the outline. When closing panel, the item is still focused, but there is no outline, that makes it seem like its not focused but it is and icons are also still visible.

Do you think we should fix the outline and make it visible after closing? focus stays on the item in that case as it was before panel was opened. So user can keep navigating using arrow keys from where they left

@JayaKrishnaNamburu JayaKrishnaNamburu mentioned this pull request Oct 31, 2023
Closed
@JayaKrishnaNamburu
Copy link
Contributor Author

JayaKrishnaNamburu commented Oct 31, 2023

@taylornowotny we moved the bug to a new issue here. And will be handled separately. #2543
Let's move the discussion on this bug there, so all the discussions will be in one place there 👍

@JayaKrishnaNamburu JayaKrishnaNamburu self-assigned this Oct 31, 2023
taylornowotny
taylornowotny approved these changes Nov 1, 2023
View reviewed changes
Copy link
Contributor

@taylornowotny taylornowotny left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Bug moved to a separate issue.

UI not implemented - I'm creating a separate issue for this. #2546

Approving just so I'm not blocking this.

@taylornowotny taylornowotny mentioned this pull request Nov 1, 2023
Closed
@JayaKrishnaNamburu JayaKrishnaNamburu merged commit 60a6a7f into main Nov 1, 2023
@JayaKrishnaNamburu JayaKrishnaNamburu deleted the add-css-transitions branch November 1, 2023 17:36
@JayaKrishnaNamburu JayaKrishnaNamburu mentioned this pull request Nov 17, 2023
Merged
2 tasks
istarkov pushed a commit that referenced this pull request Nov 17, 2023
@JayaKrishnaNamburu
chore: remove feature flag check for transitions (#2622)
f178047 
## Description

All the PR's related to transitions are merged now. 
#2581 
#2572
#2538
#2511

## Before requesting a review

- [x] made a self-review
- [x] added inline comments where things may be not obvious (the "why",
not "what")
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kof kof kof left review comments

@istarkov istarkov istarkov approved these changes

+1 more reviewer

@taylornowotny taylornowotny taylornowotny approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JayaKrishnaNamburu JayaKrishnaNamburu

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@JayaKrishnaNamburu @kof @taylornowotny @istarkov