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

Page Attachment block #3035

Merged
merged 106 commits into from
Sep 12, 2019
Merged

Page Attachment block #3035

merged 106 commits into from
Sep 12, 2019

Conversation

miina
Copy link
Contributor

@miina miina commented Aug 14, 2019

Closes #1973

Adds Page Attachment block, replicates the component's FE behavior.

Todo.

  • Add Page Attachment block.
  • Display the block in Inserter only when allowed.
  • Ensure being the last block.
  • Add CTA button which will reveal the attachment.
  • Display content based on the selected post ID.
  • Add search form for getting an actual post to display (currently static 1).
  • Decide what to display from the post.
  • Allow differet post types.
  • Add background color, font, text color settings?
  • Add proper block icon.
  • E2E
  • Refactor code

@googlebot googlebot added the cla: yes Signed the Google CLA label Aug 14, 2019
@miina
Copy link
Contributor Author

miina commented Aug 14, 2019

Started creating the Page Attachment block, while creating it, a few questions arose:

  • It seems like it would make sense to additionally allow choosing at least background color and font size in addition to just pulling in the content. Otherwise, it's either just on a dark or light background and might not look the best. An alternative would be to always display light text if using the dark theme and dark text when using the light theme. Thoughts?
  • Any other settings that might be necessary?
  • Currently, the editor design is based on how it would look like on a mobile phone. Desktop design in the FE differs quite a bit. Perhaps we should implement the desktop design instead in the editor, that should cover the landscape as well then. Thoughts?

This is how it looks like now. Light theme:
attachment

Dark theme:
Screenshot 2019-08-15 at 00 02 47

Additionally, I was searching around a bit about duplicate content and quoting and citing and only found information about quote and cite not being treated differently, meaning that even if we'd wrap all the content inside those it would still be considered duplicate. Is there any resource which would state otherwise?

Originally I suggested displaying the excerpt in attachment but now testing it out it looks like that would be too short to look good.

If the post content would be considered duplicate anyway then perhaps it could make sense to implement Inner blocks instead then? E.g. Text block and Image block to start with.

Or do you think it's OK to just pull the content even if it'll be considered duplicate?

cc @swissspidy @westonruter

@miina
Copy link
Contributor Author

miina commented Aug 14, 2019

Actually, I didn't try how the attachment would look like with all these together:

  • Title
  • Category
  • Featured Image
  • Excerpt (or perhaps X first paragraphs or X first words?)
  • Link to the Post.

Perhaps that could still work as well.

Would be great to hear your thoughts as well.

@swissspidy
Copy link
Collaborator

Love the WYSIWYG edit functionality already!

It seems like it would make sense to additionally allow choosing at least background color and font size in addition to just pulling in the content. Otherwise, it's either just on a dark or light background and might not look the best. An alternative would be to always display light text if using the dark theme and dark text when using the light theme. Thoughts?

Adding text & background color options sounds reasonable.

Currently, the editor design is based on how it would look like on a mobile phone. Desktop design in the FE differs quite a bit. Perhaps we should implement the desktop design instead in the editor, that should cover the landscape as well then. Thoughts?

Since we don't have landscape support in the editor yet, using the mobile design in the editor makes sense.

Additionally, I was searching around a bit about duplicate content and quoting and citing and only found information about quote and cite not being treated differently, meaning that even if we'd wrap all the content inside those it would still be considered duplicate. Is there any resource which would state otherwise?

Or do you think it's OK to just pull the content even if it'll be considered duplicate?

No idea off the top of my head. I wouldn't worry too much about that though. If I select a post to be displayed in that page attachment I'd write the content specific to that attachment anyway.

Originally I suggested displaying the excerpt in attachment but now testing it out it looks like that would be too short to look good.

I'd assume one would want to tweak the template that is used for that anyway.

Or perhaps there could be options like "display excerpt", "display full content", "display featured image", "display read more link", etc.

If the post content would be considered duplicate anyway then perhaps it could make sense to implement Inner blocks instead then? E.g. Text block and Image block to start with.

I'm not sure how big of a hassle that would be. It would require us to go back to a linear edit flow for the attachment, showing block movers, disallow dragging, etc.

Long term we'd probably want to support that anyway though, soo... 🤷‍♂

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Sep 12, 2019
@spacedmonkey
Copy link
Contributor

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Sep 12, 2019
@spacedmonkey spacedmonkey force-pushed the add/1973-page_attachment branch from 9e84b09 to 86bdb54 Compare September 12, 2019 17:08
@swissspidy swissspidy merged commit 33f3d7c into develop Sep 12, 2019
@swissspidy swissspidy deleted the add/1973-page_attachment branch September 12, 2019 17:36
westonruter added a commit that referenced this pull request Sep 14, 2019
…keep-attributes-with-mustache-placeholders-intact

* 'develop' of github.com:ampproject/amp-wp: (113 commits)
  This converts the keyboard cut handler to equal a copy handler to avoid bugs
  Fix aria-label adding helper function
  Remove extra line I added in resolving merge conflict
  Fix alignment of arrow
  Fix custom CTA text for page attachment
  Fix cut handler by shortcut
  Cleanup of duplicated comment
  Add unit testing to `addVideoAriaLabel`
  Remove unused piece of code
  Remove Cloudflare AMP cache since deprecated
  Handle cut keyboard requests. (#3231)
  Page Attachment block (#3035)
  Keyboard & Right-Click Menu Copy + Paste (#3083)
  Animation Previews (#3104)
  Make internal methods inaccessible
  Omit the ecosystem link also when using a core theme
  Add skipped e2e test for the video block
  Add array_colum() pollyfill for PHP < 5.5
  Add asserts to make sure we are not enqueueing both versions of dashicons
  Remove useless variable
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement support for <amp-story-page-attachment>
7 participants