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

Try: Provider-specific embed block settings (Tweet theme option) #3032

Closed
wants to merge 4 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Oct 16, 2017

Related: #2744

This pull request seeks to explore adding provider-specific embed block settings. Many oEmbed providers support additional URL parameters which are not part of the embedded URL itself, but can be passed as part of the oEmbed fetch request. It is not currently possible to pass additional arguments through as part of this fetch request except through filters. The changes here seek to introduce support for an args shortcode argument on the underlying [embed] shortcode, and complementary block controls to control their setting. This is currently implemented in an isolated case, exposing the Twitter embed "Dark theme" display:

Twitter dark theme

Tweet oEmbed parameter reference: https://developer.twitter.com/en/docs/tweets/post-and-engage/api-reference/get-statuses-oembed#parameters

Implementation notes:

The implementation warrants a second look and is incomplete:

  • The embed preview in the editor does not reflect added arguments because it does not yet include these attributes in the preview request (unsure yet whether the oEmbed proxy endpoint will support this)
  • Is there a reason we wouldn't want to allow additional arguments to be added to the oEmbed fetch URL? At least in this case, it isn't possible to pass through ad-hoc values, since it is specifically checking the value of theme and assigning 'dark' explicitly.
  • There is duplication in how the theme attribute is assigned, since the server-defined attributes are not recursively merged. A better long-term solution here is consolidating attributes definition to the server (Block API: Server-side awareness of block types #2751).
  • We might want to think of a better way to define embed block inspector settings, either auto-scaffolding a form from additional block attributes, or by refactoring the current getEmbedBlockSettings function into a more extendable / composable format.
  • We may want to introduce the 'enum' type for block attributes, which is already supported in the REST API (whose functions are used for server-side attribute validation) and could simplify the definition of a predefined set of values ('light', 'dark')

Testing instructions:

  1. Navigate to Gutenberg > New Post
  2. Insert a Twitter embed block
  3. Add a URL to the embed placeholder block
  4. Press the ellipsis toggle next to the block preview (selecting block is otherwise difficult?)
  5. Select "Settings"
  6. Note the "Dark Theme" checkbox
  7. Toggle the "Dark Theme" checkbox
  8. Press the Preview icon button in the editor header

@aduth aduth added [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress labels Oct 16, 2017
@codecov
Copy link

codecov bot commented Oct 16, 2017

Codecov Report

Merging #3032 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3032      +/-   ##
==========================================
- Coverage   32.76%   32.75%   -0.02%     
==========================================
  Files         203      203              
  Lines        5951     5953       +2     
  Branches     1052     1053       +1     
==========================================
  Hits         1950     1950              
- Misses       3375     3376       +1     
- Partials      626      627       +1
Impacted Files Coverage Δ
blocks/library/embed/index.js 44.56% <0%> (-1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 588ccd3...f803f23. Read the comment docs.

@danielbachhuber
Copy link
Member

@aduth Given this isn't a blocker for Merge Proposal, can we close in favor of its issue? #2744

@aduth
Copy link
Member Author

aduth commented Mar 30, 2018

Yeah, this has languished long enough.

I think @notnownikki had at one point expressed interest in championing this, though I doubt it would be done as a direct continuation of this branch.

@aduth aduth closed this Mar 30, 2018
@aduth aduth deleted the try/embed-block-settings branch March 30, 2018 15:40
@notnownikki
Copy link
Member

Sorry, this had been in the back of my mind but I'd been concentrating on the other embed issues before I moved on to this. I still think it's a great step forward so I've the other embed issues are closed I'll be happy to move on to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants