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

EWPP-82: Allow to configure a text format for the video iframe source. #117

Merged
merged 30 commits into from
Aug 6, 2020

Conversation

sergepavle
Copy link
Member

OPENEUROPA-[Insert ticket number here]

Description

[Insert description here]

Change log

  • Added:
  • Changed:
  • Deprecated:
  • Removed:
  • Fixed:
  • Security:

Commands

[Insert commands here]

*/
public function testMediaSourceTextFormats(): void {
foreach ($this->getFixtures() as $test_data) {
$media = $this->storage->create([
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better if you create a new media type with the iframe source to make sure the text format works for any media type instead of testing the video iframe type.

Copy link
Member Author

@sergepavle sergepavle Jul 17, 2020

Choose a reason for hiding this comment

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

@brummbar What do you think should we build media type programmatically? Do we have plans that this particular media source could be used in other media types?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it will be used in 1 more at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

*/
function oe_media_iframe_post_update_00001() {
$format = \Drupal::entityTypeManager()->getStorage('filter_format')->create([
'format' => 'oe_media_iframe',
Copy link
Contributor

Choose a reason for hiding this comment

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

In theory, this should not exist but if the format "oe_media_iframe" was created by someone using this component, it will fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

*/
public function testMediaSourceTextFormats(): void {
foreach ($this->getFixtures() as $test_data) {
$media = $this->storage->create([
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it will be used in 1 more at least.

/**
* Test text formats selected for Media source.
*/
public function testMediaSourceTextFormats(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make one test for the widget configuration that gets reflected in the backend. One kernel test that checks the formatter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@sergepavle sergepavle force-pushed the EWPP-82 branch 2 times, most recently from af834ed to 95d6ac6 Compare July 30, 2020 16:47
$media_type = $this->entityTypeManager->getStorage('media_type')->load($this->fieldDefinition->getTargetBundle());
$text_format = $media_type->getSource()->getConfiguration()['text_format'] ?? NULL;

$element = $main_widget['value'];
Copy link
Contributor

Choose a reason for hiding this comment

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

@sergepavle The problem is that you are only returning the 'value' of the main widget. You add to that and return that but the problem is you then lose the top level array inside $main_widget

@upchuk upchuk merged commit b7019f6 into EPIC-video-iframe-media-v2 Aug 6, 2020
@upchuk upchuk deleted the EWPP-82 branch August 6, 2020 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants