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

Use nested blocks for quotes #6520

Closed
wants to merge 2 commits into from
Closed

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented May 1, 2018

Revert "Revert "Use nested blocks for quotes (#6054)" (#6501)"
This reverts commit 230ad0d.

Description

Redo of #6054.

Things to fix:

  • Formatting toolbar for cite.
  • Cite placeholder is hard to discover.
  • There should be no arrows if there is only one block.

How has this been tested?

Create a quote and pull quote block.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@@ -82,7 +82,7 @@

.block-rich-text__inline-toolbar {
display: flex;
justify-content: center;
justify-content: attr( data-justify-content );
Copy link
Member Author

Choose a reason for hiding this comment

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

@jasmussen I don't know if this is a good way to address the issue where the toolbar should be aligned the same as the content? Looking at the text-align property seems too hard to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit hard to grok for me without a screenshot. I'm also not familiar with the justify-content property. Is this about the alignment of the inline toolbar in context of that text being non-centered?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasmussen Sorry, I changed it in the meantime because this didn't work... I've added the styles inline in React now. Ideally the toolbar should be positioned the same as the text, but it doesn't seem like there's an easier way to do it. The block has to set the toolbar position as well.

@tofumatt
Copy link
Member

There are a lot of conflicts with this branch, I'm guessing because it's a bit old. Could you rebase onto master before we get to reviewing, just in case the conflict resolution changes anything non-trivial?

@ellatrix
Copy link
Member Author

Rebasing...

@ellatrix ellatrix force-pushed the try/quote-nested-blocks branch from 56315f1 to 797b221 Compare June 20, 2018 17:03
@ellatrix
Copy link
Member Author

Oh, it's again out of date.

Migrations stopped working for some reason, looking into it, and fixing e2e tests.

@ellatrix ellatrix force-pushed the try/quote-nested-blocks branch 2 times, most recently from 2555cc3 to 39c7a79 Compare June 21, 2018 10:52
@ellatrix
Copy link
Member Author

Once more out of date :)

@ellatrix ellatrix force-pushed the try/quote-nested-blocks branch from 39c7a79 to b49cef0 Compare June 21, 2018 11:54
@ellatrix
Copy link
Member Author

Discovered by an e2e test (yay): The in-between inserter is odd at the edges of a nested block areas. It's not clear where the block should be inserted: above the wrapper or inside the wrapper. For users this will get a bit confusing. Cc @jasmussen @karmatosed.

@ellatrix ellatrix force-pushed the try/quote-nested-blocks branch from b49cef0 to 2dcba4d Compare June 21, 2018 13:47
@jasmussen
Copy link
Contributor

Discovered by an e2e test (yay): The in-between inserter is odd at the edges of a nested block areas. It's not clear where the block should be inserted: above the wrapper or inside the wrapper. For users this will get a bit confusing.

This is the new version, correct?

Can you elaborate a bit on this? Is it a visual issue or a technical one?

@ellatrix
Copy link
Member Author

This is the new version, correct?

Yes, current master.

Can you elaborate a bit on this? Is it a visual issue or a technical one?

Visual/UX. There are two possible insertion points at one single line at the edges of nested block areas.

@ellatrix ellatrix force-pushed the try/quote-nested-blocks branch from 2dcba4d to d9ad0d5 Compare June 25, 2018 17:45
@ellatrix ellatrix force-pushed the try/quote-nested-blocks branch from d9ad0d5 to cc659d6 Compare June 25, 2018 17:56
@ZebulanStanphill
Copy link
Member

@iseulde @jasmussen The Divi Visual Builder gets around this issue by shifting the inserter buttons to the left or right when they get too close together and by having different colors for the inserter buttons depending on whether something is a section, row, or module. Not exactly sure how something like this would be implemented in Gutenberg, but I thought I would point it out anyway in case it gives any ideas:

https://supergeniuszeb.com/wp-content/uploads/2018/06/Divi-Visual-Builder-add-button-responsiveness-demonstration.webm

@ellatrix ellatrix added this to the 3.2 milestone Jun 27, 2018
@mcsf mcsf modified the milestones: 3.2, 3.3 Jul 5, 2018
@aduth aduth removed this from the 3.3 milestone Jul 18, 2018
@pento pento added this to the 3.5 milestone Jul 30, 2018
@ZebulanStanphill
Copy link
Member

@iseulde @jasmussen The sibling inserter issue could be resolved if something like these mockups I made in #6224 was implemented.

image

This would make it very clear where a block would be inserted, since the sibling inserter would be more closely tied to the block it is inserting after. There would also be no overlapping sibling inserters like there are now.

@ZebulanStanphill
Copy link
Member

I proposed a possible solution to the overlapping sibling inserters issue in #9202.
image

Note also the improvements to the block UI proposed in #9074 and #9282 that would make it easier to interact with the blocks nested in a Quote block.

@tofumatt
Copy link
Member

tofumatt commented Dec 3, 2018

Because #6054 was merged, I'm going to close this. It looks like this should've been closed but it slipped through the cracks.

@tofumatt tofumatt closed this Dec 3, 2018
@tofumatt tofumatt deleted the try/quote-nested-blocks branch December 3, 2018 16:23
@ellatrix ellatrix restored the try/quote-nested-blocks branch December 3, 2018 17:23
@ellatrix
Copy link
Member Author

ellatrix commented Dec 3, 2018

@tofumatt That commit was reverted.

@ellatrix ellatrix reopened this Dec 3, 2018
@tofumatt
Copy link
Member

tofumatt commented Dec 3, 2018

Shoot, I see, my bad 😓

Sorry, I was just trying to clear up old PRs still flagged for review. Does this still require a review then?

@ellatrix
Copy link
Member Author

ellatrix commented Dec 3, 2018

I think this just needs another rebase, but it seems less important atm.

@tofumatt
Copy link
Member

tofumatt commented Dec 3, 2018

Fair enough, sorry I was working backward through my review queue so no rush 😅

@gziolo gziolo added [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Block] Quote Affects the Quote Block [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P labels Jan 29, 2019
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This one needs a solid rebase to bring it up to date. @youknowriad - is this something we want to spend time on in Phase 2? Does it make sense to refresh it or should we rather close it and reopen in the future when it's necessary? I'm marking it as Stale until it will get revisited.

@Sven74Muc
Copy link

I'm pointed here through this ticket: https://core.trac.wordpress.org/ticket/46341

Any chance to go ahead with this issue? For me it looks like it's stil open but no work on it.

(I'm a newbee here on github, so please excuse if I missunderstand this thread.)

@ellatrix
Copy link
Member Author

@Sven74Muc Yes, this pull request (branch) should be rebased or we could also start from scratch. Last time I checked the code looked good, but there were some issues with the design of nested blocks that made us want to wait with this until they are resolved. I'm not sure if this is still the case. Cc @jasmussen or @karmatosed? Do you think we can move forward with using nested blocks in the quote blocks?

@Sven74Muc
Copy link

Sven74Muc commented Feb 26, 2019

@iseulde For me it is not only about nested a bullet list within quote. There are also other blocks like table or column which could be nested in a quot. On the other side ther could also be a bullet list nested in a table or column for example. So.... column for example (or other) could be nested somewhere else or other way arround other things could be nested within column (or other)

"using nested block in a quote block" AND nest a quote block in other blocks like table.

Think it makes sense not just to go for one case, it should be an in all directions flexible solution. What do you think?

@Sven74Muc
Copy link

@jasmussen @karmatosed @iseulde For me it looks with the bigger appraoch (not just nested blocks within quotes) that it makes sens to start from scratch. Maybe one approach ist like with the header block... there you have the align functionality within the right sitebar... So not a nested approach, more a seperated approch.

@draganescu
Copy link
Contributor

@ellatrix should we then close this and redo the whole thing? By the looks of the conflict list it would be a horror story to rebase :D

@ellatrix
Copy link
Member Author

Sure :)

@ellatrix ellatrix closed this Apr 23, 2019
@ellatrix ellatrix deleted the try/quote-nested-blocks branch April 23, 2019 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Quote Affects the Quote Block [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants