Skip to content

Reordering page thumbnails and the grid view#803

Merged
wassgha merged 18 commits intomasterfrom
refactor/reorder-thumbnails
Apr 10, 2020
Merged

Reordering page thumbnails and the grid view#803
wassgha merged 18 commits intomasterfrom
refactor/reorder-thumbnails

Conversation

@wassgha
Copy link
Copy Markdown
Contributor

@wassgha wassgha commented Mar 26, 2020 •
edited
Loading

Closes #746
Closes #332
Closes #517

Uses the same generalized <Reorderable/> component in the layers panel, the page thumbnails and the grid view
/cc @dvoytenko

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 26, 2020 •
edited
Loading

Size Change: -2.44 kB (0%)

Total Size: 868 kB

Filename Size Change
assets/js/edit-story.js 444 kB +325 B (0%)
assets/js/stories-dashboard.js 418 kB -2.76 kB (0%)
ℹ️ View Unchanged
Filename Size Change
assets/css/edit-story.css 3.04 kB 0 B
assets/css/stories-dashboard.css 3.01 kB 0 B

compressed-size-action

@wassgha wassgha force-pushed the refactor/reorder-thumbnails branch from e17a388 to 314276d Compare March 30, 2020 07:59
@swissspidy swissspidy added the Type: Enhancement New feature or improvement of an existing feature label Mar 30, 2020
@wassgha wassgha mentioned this pull request Mar 30, 2020
Closed
@wassgha wassgha marked this pull request as ready for review April 1, 2020 10:00
@wassgha
Copy link
Copy Markdown
Contributor Author

wassgha commented Apr 1, 2020

@dvoytenko @barklund this is ready for review. It seems like we might have overcomplicated the scrolling behavior in #551 , a simple separator.scrollIntoView() works pretty well here!

@wassgha wassgha mentioned this pull request Apr 1, 2020
Merged
swissspidy
swissspidy reviewed Apr 1, 2020
View reviewed changes
assets/src/edit-story/components/reorderable/reorderableSeparator.js Outdated
} = useReorderable();
const handlePointerEnter = useCallback(() => {
setCurrentSeparator(position);
separatorRef.current.scrollIntoView({
Copy link
Copy Markdown
Collaborator

@swissspidy swissspidy Apr 1, 2020

Choose a reason for hiding this comment

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

Safari only supports scrollIntoView with boolean parameter, but not smooth behavior option.

Copy link
Copy Markdown
Contributor Author

@wassgha wassgha Apr 2, 2020

Choose a reason for hiding this comment

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

That should be alright, just a slightly worse experience. Otherwise we can revert back to @barklund 's approach which should work for all browsers

merapi
merapi suggested changes Apr 1, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@merapi merapi left a comment

Choose a reason for hiding this comment

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

Also, noticed 3 warnings in the console.

dvoytenko
dvoytenko suggested changes Apr 1, 2020
View reviewed changes
@wassgha
Copy link
Copy Markdown
Contributor Author

wassgha commented Apr 2, 2020

If we're good with using scrollIntoView for the scroll behavior, this should be good to go

swissspidy
swissspidy reviewed Apr 2, 2020
View reviewed changes
@swissspidy swissspidy requested a review from dvoytenko April 2, 2020 12:03
dvoytenko
dvoytenko approved these changes Apr 2, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@dvoytenko dvoytenko left a comment

Choose a reason for hiding this comment

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

LG with one nit.

@dvoytenko
Copy link
Copy Markdown
Contributor

dvoytenko commented Apr 2, 2020

LG with one nit.

Though I don't have a strong opinion on scrollIntoView. IMHO it's ok, but @barklund might feel different.

@swissspidy
Copy link
Copy Markdown
Collaborator

swissspidy commented Apr 3, 2020

There is a couple of related warnings in the console:

Received NaN for the `width` attribute. If this is expected, cast the value to a string.
    in div (created by reorderableSeparator__Wrapper)

...

Warning: Each child in a list should have a unique "key" prop.
Check the render method of `Carousel`. See https://fb.me/react-warning-keys for more information.
...

@swissspidy swissspidy self-requested a review April 3, 2020 12:47
@wassgha wassgha requested a review from merapi April 4, 2020 04:34
@wassgha
Copy link
Copy Markdown
Contributor Author

wassgha commented Apr 4, 2020

@swissspidy @merapi ready :)

@merapi
Copy link
Copy Markdown
Contributor

merapi commented Apr 5, 2020

The "dead zone" is still there?

image

@wassgha
Copy link
Copy Markdown
Contributor Author

wassgha commented Apr 7, 2020

@merapi Sorry, I refactored and totally forgot about that. Should be fixed now :))

@merapi
Copy link
Copy Markdown
Contributor

merapi commented Apr 7, 2020

@merapi Sorry, I refactored and totally forgot about that. Should be fixed now :))

Can you check the Grid view? I think we need a fix there too.

dvoytenko
dvoytenko approved these changes Apr 7, 2020
View reviewed changes
barklund
barklund previously requested changes Apr 8, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@barklund barklund left a comment

Choose a reason for hiding this comment

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

A few minor bugs.

@barklund
Copy link
Copy Markdown
Contributor

barklund commented Apr 8, 2020

A couple more bugs specifically related to grid view (but not directly related to any line of code as far as I can see):

  1. You cannot scroll in the modal, even if you in the default view can't see the all the pages (I have 11):
    no-scroll

  2. Even without dragging anything, the reordering separators show up at the ends of the rows:
    weird-hover

  3. As you reorder, there's no separator before the first item in the second (and subsequent) rows:
    missing-2nd-row-sep

I don't know if they're out of scope for this ticket, though. 1. is potentially a general modal bug (with a potentially simple fix).

@wassgha
Copy link
Copy Markdown
Contributor Author

wassgha commented Apr 9, 2020

Did a refactor (went back to using @barklund 's scroller approach since it was smoother) and fixed all of those issues :)

@wassgha wassgha requested a review from barklund April 9, 2020 19:44
barklund
barklund reviewed Apr 9, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@barklund barklund left a comment

Choose a reason for hiding this comment

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

This works really well now - great job. There's just a single bug with the first separator missing in the carousel when it's scrollable.

@wassgha wassgha requested a review from barklund April 10, 2020 08:10
@merapi
Copy link
Copy Markdown
Contributor

merapi commented Apr 10, 2020

LG!

Just one thing:
I can see the scrollbar (when not needed) in Gridview (Chrome/FF) and I think its style should match other scrollbars.
image

@wassgha wassgha dismissed barklund’s stale review April 10, 2020 19:07

barklund is out but addressed all requested changes

@wassgha wassgha merged commit 0a10023 into master Apr 10, 2020
@wassgha wassgha deleted the refactor/reorder-thumbnails branch April 10, 2020 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@swissspidy swissspidy Awaiting requested review from swissspidy

@barklund barklund Awaiting requested review from barklund

2 more reviewers

@dvoytenko dvoytenko dvoytenko approved these changes

@merapi merapi merapi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Type: Enhancement New feature or improvement of an existing feature

Projects

None yet

Milestone

No milestone

6 participants

@wassgha @dvoytenko @swissspidy @merapi @barklund @googlebot