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

PLANET-4811 Submenu WYSIWYG #328

Closed
wants to merge 38 commits into from
Closed

PLANET-4811 Submenu WYSIWYG #328

wants to merge 38 commits into from

Conversation

mleray
Copy link
Contributor

@mleray mleray commented Jul 16, 2020

See https://jira.greenpeace.org/browse/PLANET-4811

  • Turn the LayoutSelector into block styles
  • Turn the Title field into a RichText component
  • Move level options to the Sidebar

More details in the doc

@mleray mleray added the WIP label Jul 16, 2020
@mleray mleray force-pushed the planet-4811 branch 7 times, most recently from d771ede to 6216d38 Compare July 20, 2020 14:33
@mleray mleray force-pushed the planet-4811 branch 4 times, most recently from 5886583 to 382cb80 Compare July 28, 2020 15:12
mleray
mleray commented Jul 28, 2020
View reviewed changes
classes/command/class-shortcode-to-gutenberg.php
@@ -32,15 +32,13 @@ public function init() {
'shortcake_columns',
'shortcake_carousel_header',
'shortcake_cookies',
'shortcake_counter',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this one here too because it was forgotten when converting Counter to WYSIWYG 😬

@mleray mleray added Review and removed WIP labels Jul 29, 2020
@mleray mleray requested review from a team, pablocubico and sagarsdeshmukh and removed request for a team July 29, 2020 08:15
@mleray mleray force-pushed the planet-4811 branch 4 times, most recently from 2f8d0fb to 93e130d Compare July 31, 2020 12:03
@mleray
Copy link
Contributor Author

mleray commented Aug 7, 2020

There is another open PR for this issue, to remove the endpoint: #356

@mleray
Copy link
Contributor Author

mleray commented Aug 11, 2020 •
edited
Loading

Copying here @sagarsdeshmukh's comment from the other PR:

Functionality wise the new wysiwyg submenu block looks good to me 👍
Found few observations -

  • The transition to move focus from clicked submenu link to actual heading is different from old submenu block (in old submenu block it was slow, In new one its fast)
  • If heading title contains a special char's like apostrophe the submenu link won’t works
  • The heading title got hide under top menu bar when scrolled to link

https://drive.google.com/file/d/1t_oVWunSj1pyCXS-YnND4lqjbAhoVHn6/view?usp=sharing

Inwerpsel
Inwerpsel reviewed Aug 11, 2020
View reviewed changes
assets/src/blocks/Submenu/useSubmenuItemsLoad.js
useEffect(() => {
const items = loadMenuItems();
setMenuItems(items);
}, [levels]);
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 actually a bit more tricky, the block also needs to re-render if any headings are added, removed or changed on the page. Not sure what would be the best way. We could do a document.querySelectorAll and pass the result as a dependency to useEffect, but that would be a bit duplicate logic with what happens inside loadMenuItems

mleray added 24 commits August 12, 2020 11:55
@mleray
PLANET-4811 Add missing translations
a69e221
@mleray
PLANET-4811 Add recursive function to get menu items
bc6d1e2
@mleray
PLANET-4811 Remove old conversion code
8f852bd
@mleray
PLANET-4811 Add proper links and back-to-top behavior
4707554
@mleray
PLANET-4811 Fix Submenu title color in the editor
981aeda
@mleray
PLANET-4811 Fix deprecated attributes
0a02c67
@mleray
PLANET-4811 Add target links only when needed
8391c5e
@mleray
PLANET-4811 Fix Submenu rendering
205fdee
@mleray
PLANET-4811 Simplify style registration
2e9c884
@mleray
PLANET-4811 Remove jQuery code
2dee29b
@mleray
PLANET-4811 Use wp.blockEditor instead of wp.editor to fix console wa…
c64389a 
…rning
@mleray
PLANET-4811 Remove css float in the editor
629d9e7
@mleray
PLANET-4811 Create single file for all submenu functions
fb74aca
@mleray
PLANET-4811 Remove unused SubmenuIcon
db787b8
@mleray
PLANET-4811 Improve code after review
f2f9669
@mleray
PLANET-4811 Remove endpoint and retrieve content in the frontend instead
56c7811
@mleray
PLANET-4811 Remove md5
025ef07
@mleray
PLANET-4811 Remove now unused postId prop
73f5cbe
@mleray
PLANET-4811 Add children behaviour to loadMenuItems function
701d75b
@mleray
PLANET-4811 Improve code after review
90a26e9
@mleray
PLANET-4811 Create SubmenuItems component and useSubmenuItemsLoad hook
93cdb56
@mleray
PLANET-4811 Fix back to top button positioning
be64e68
@mleray
PLANET-4811 Make sure ids are unique
9af7a0c
@mleray
PLANET-4811 Fix scrolling animation
b21b331
@sagarsdeshmukh
Copy link
Member

Copying here @sagarsdeshmukh's comment from the other PR:

Functionality wise the new wysiwyg submenu block looks good to me 👍
Found few observations -

  • The transition to move focus from clicked submenu link to actual heading is different from old submenu block (in old submenu block it was slow, In new one its fast)
  • If heading title contains a special char's like apostrophe the submenu link won’t works
  • The heading title got hide under top menu bar when scrolled to link

https://drive.google.com/file/d/1t_oVWunSj1pyCXS-YnND4lqjbAhoVHn6/view?usp=sharing

Hi @mleray - Sorry for the delay in reply, I tested the latest changes at here and above issues seems to fixed including the special char issue as well.
Just one thing I found, the title of submenu block is display as a first link in submenu. other than that functionality wise 👍 .

@mleray
Copy link
Contributor Author

mleray commented Aug 26, 2020

Copying here @sagarsdeshmukh's comment from the other PR:
Functionality wise the new wysiwyg submenu block looks good to me 👍
Found few observations -

  • The transition to move focus from clicked submenu link to actual heading is different from old submenu block (in old submenu block it was slow, In new one its fast)
  • If heading title contains a special char's like apostrophe the submenu link won’t works
  • The heading title got hide under top menu bar when scrolled to link

https://drive.google.com/file/d/1t_oVWunSj1pyCXS-YnND4lqjbAhoVHn6/view?usp=sharing

Hi @mleray - Sorry for the delay in reply, I tested the latest changes at here and above issues seems to fixed including the special char issue as well.
Just one thing I found, the title of submenu block is display as a first link in submenu. other than that functionality wise 👍 .

Thank you for testing again! There is yet another PR open for this issue 🙈 And the title being displayed as a link is taken care of there

@Inwerpsel
Copy link
Contributor

Closing this as we'll continue with #363, which already has the commits from this PR

@Inwerpsel Inwerpsel closed this Sep 21, 2020
@mleray mleray deleted the planet-4811 branch October 12, 2020 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Inwerpsel Inwerpsel Inwerpsel left review comments

@pablocubico pablocubico Awaiting requested review from pablocubico

@sagarsdeshmukh sagarsdeshmukh Awaiting requested review from sagarsdeshmukh

Assignees
No one assigned
Labels
Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mleray @sagarsdeshmukh @Inwerpsel