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

MediaEmbed: Override custom element names #9418

Closed
wants to merge 12 commits into from
Closed

MediaEmbed: Override custom element names #9418

wants to merge 12 commits into from

Conversation

tony
Copy link
Contributor

@tony tony commented Apr 3, 2021

Alternative to #9373

  1. elementNames removed from public API
  2. oembed works implicitly, even when preferredElementName is custom

See #9375 (comment)

This was referenced Apr 3, 2021
Closed
Closed
@tony tony changed the title MediaEmbed: Rename <oembed> to <o-embed> for custom elements compatibility (alt) MediaEmbed: Set custom element name (alt) Apr 3, 2021
psmyrek
psmyrek requested changes Apr 6, 2021
View reviewed changes
@tony tony force-pushed the tn-mediaembed-element-name-alt branch from d6dfdf2 to d6f2c91 Compare April 6, 2021 13:27
@tony
Copy link
Contributor Author

tony commented Apr 6, 2021 •
edited
Loading

@psmyrek Updated

Thank you for the review!

Would you like me to squash all the commits together? Okay if I squash your commits into mine too?

psmyrek
psmyrek requested changes Apr 7, 2021
View reviewed changes
Copy link
Contributor

@psmyrek psmyrek left a comment

Choose a reason for hiding this comment

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

Thanks for fixes so far, your PR looks very good!

I've found a few minor, mostly cosmetic issues and then your PR will be ready.

There's no need to squash commits together.

@tony tony force-pushed the tn-mediaembed-element-name-alt branch from d6f2c91 to 780250c Compare April 8, 2021 00:05
oleq
oleq requested changes Apr 8, 2021
View reviewed changes
Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

LGTM, just a few remarks 👍

packages/ckeditor5-media-embed/docs/features/media-embed.md Outdated
</figure>
```

To be backward compatible with legacy semantic elements, `<oembed>` tags are still supported when `elementName` is set.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this sentence. The <oembed> will be used when elementName is set to what?

module:media-embed/mediaembed~MediaEmbedConfig#elementName suggests that <oembed> is used when elementName is unset, so I think the above contradicts the API docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if elementName is overridden, oembed will still be upcasted for backward compatibility purposes.

I'll rewrite the sentence to see if it makes more sense :)

packages/ckeditor5-media-embed/src/mediaembed.js Outdated
@@ -236,6 +236,24 @@ export default class MediaEmbed extends Plugin {
* @member {Array.<String>} module:media-embed/mediaembed~MediaEmbedConfig#removeProviders
*/

/**
* Customizing semantic element name.
Copy link
Member

Choose a reason for hiding this comment

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

This documentation should link to previewsInData and explain the relation between these two (one does not make sense when the other is set true).

packages/ckeditor5-media-embed/src/mediaembed.js
* </figure>
*
* @member {String} [module:media-embed/mediaembed~MediaEmbedConfig#elementName]
*/
Copy link
Member

Choose a reason for hiding this comment

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

Missing @default '???'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@default 'oembed' I presume?

@psmyrek
Copy link
Contributor

psmyrek commented Apr 8, 2021

Thanks @oleq for your review.
@tony Would you like to add these final changes?

tony and others added 12 commits April 8, 2021 09:37
@tony
Internal: Allow setting semantic element name in Media Embeds, fixes c…
863592d 
…keditor#9373
@tony
Test: Update for 27.0.0
3021fbf
@tony
Make elementNames private
270ac31
@tony @psmyrek
Update packages/ckeditor5-media-embed/src/mediaregistry.js
a04d925 
Co-authored-by: Paweł Smyrek <56868128+psmyrek@users.noreply.github.com>
@tony @psmyrek
Update packages/ckeditor5-media-embed/src/mediaembed.js
18ce5a0 
Co-authored-by: Paweł Smyrek <56868128+psmyrek@users.noreply.github.com>
@tony @psmyrek
Update packages/ckeditor5-media-embed/docs/features/media-embed.md
ef474e5 
Co-authored-by: Paweł Smyrek <56868128+psmyrek@users.noreply.github.com>
@tony
Rename preferredElementName -> elementName
ff060ab
@tony
Remove unused branch
236578e
@tony
Remove elementNames
fca6070
@tony
Add documentation and fix new line
8d2e5a4
@tony
Remove unused elementName instance variable
f08a3d2
@tony
Update documentation
347c786
@tony tony force-pushed the tn-mediaembed-element-name-alt branch from 84aa3c0 to 347c786 Compare April 8, 2021 14:38
@tony
Copy link
Contributor Author

tony commented Apr 8, 2021

@psmyrek @oleq: Updated! How is this? Let me know if there's anything I clear up / rewrite.

This is my first adding documentation, so I'm still getting acclimated with the tone/style used.

psmyrek added a commit that referenced this pull request Apr 12, 2021
@psmyrek
Merge pull request #9418 from tony/tn-mediaembed-element-name-alt
aefc6a2 
Feature (media-embed): Introduced the `config.mediaEmbed.elementName` to allow setting semantic element name. Closes #9373.
@psmyrek
Copy link
Contributor

psmyrek commented Apr 12, 2021

Hi @tony, thanks for your contribution! I've just merged your changes in aefc6a2.

@psmyrek psmyrek closed this Apr 12, 2021
@tony tony deleted the tn-mediaembed-element-name-alt branch April 12, 2021 12:40
@tony
Copy link
Contributor Author

tony commented Apr 12, 2021

@psmyrek Thank you!

@tony tony changed the title MediaEmbed: Set custom element name (alt) MediaEmbed: Override custom element names Apr 12, 2021
@tony tony restored the tn-mediaembed-element-name-alt branch June 21, 2021 16:03
@tony tony mentioned this pull request Nov 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleq oleq oleq requested changes

@psmyrek psmyrek psmyrek approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@tony @psmyrek @oleq