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

Draggable Block Navigation #2244

Merged
merged 10 commits into from
May 3, 2019
Merged

Draggable Block Navigation #2244

merged 10 commits into from
May 3, 2019

Conversation

swissspidy
Copy link
Collaborator

Fixes #1974.

@googlebot googlebot added the cla: yes Signed the Google CLA label May 2, 2019
@swissspidy
Copy link
Collaborator Author

swissspidy commented May 3, 2019 •
edited
Loading

There's one thing left here:

The position of the element's clone is completely off, probably because the block navigation itself is already positioned absolutely on the screen, which throws off the calculation.

Screenshot:

Edit: Resolved now.

@swissspidy swissspidy marked this pull request as ready for review May 3, 2019 11:06
miina
miina reviewed May 3, 2019
View reviewed changes
Copy link
Contributor

@miina miina left a comment

Choose a reason for hiding this comment

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

Works well. Some minor comments about styling.

assets/src/blocks/amp-story-page/edit.css Show resolved Hide resolved
assets/src/components/block-navigation/edit.css Show resolved Hide resolved
@swissspidy
Use grab cursor while dragging
1ab0cc5
@swissspidy
Make draggable clone selector more specific
fc5b22c 
It should only apply to blocks in the block list, not other usages of draggable
@swissspidy swissspidy requested a review from miina May 3, 2019 15:28
miina
miina approved these changes May 3, 2019
View reviewed changes
Copy link
Contributor

@miina miina left a comment

Choose a reason for hiding this comment

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

LGTM.

@swissspidy swissspidy merged commit a878546 into amp-stories-redux May 3, 2019
@swissspidy swissspidy mentioned this pull request May 6, 2019
Closed
@westonruter westonruter added this to the v1.2 milestone May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miina miina miina approved these changes

@westonruter westonruter Awaiting requested review from westonruter

@kienstra kienstra Awaiting requested review from kienstra

Assignees
No one assigned
Labels
cla: yes Signed the Google CLA
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.
4 participants
@swissspidy @miina @westonruter @googlebot