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

OPENEUROPA-3295: Create the video iframe media type #106

Merged
merged 15 commits into from
Jun 11, 2020

Conversation

brummbar
Copy link
Contributor

No description provided.

* @MediaSource(
* id = "oe_media_iframe",
* label = @Translation("Iframe"),
* description = @Translation("Use iframes as source for medias."),
Copy link
Contributor

Choose a reason for hiding this comment

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

Use iframes as source for media entities.

* {@inheritdoc}
*/
public function getMetadataAttributes() {
return [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we bypassing all the meta attributes? Like the parent provides for the default_name attribute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The getMetadataAttributes() method is not implemented in the parent class, while the getMetadata() is. We don't support any attribute for now, so we agreed to leave the implementation empty.

* @FieldFormatter(
* id = "oe_media_iframe",
* label = @Translation("Media iframe"),
* description = @Translation("Renders the iframe for medias with iframe sources."),
Copy link
Contributor

Choose a reason for hiding this comment

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

..for media entities

return TRUE;
}

return parent::isApplicable($field_definition);
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, not exactly. The parent returns TRUE so that means that if it's a media but with another source, we also return true. We should return false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opsie! I change it back to a return $media_type &&... like it was.

$field = $this->getSession()->getPage()->find('named', ['select', $select]);

if (empty($field)) {
throw new \Exception("Select field '{$select}' not found.");
Copy link
Contributor

Choose a reason for hiding this comment

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

why you no sprintf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I ©️ 🍝 from EWPP ihih!

Feature: Video iframe media.
In order to show remote videos
As a site editor
I want to be able to create medias with iframe as source.
Copy link
Contributor

Choose a reason for hiding this comment

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

media entities

Given I am logged in as a user with the "create oe_media_demo content, create video_iframe media" permissions
When I go to "the Video iframe creation page"
Then I should see the heading "Add Video iframe"
And the available options in the "Ratio" select should be:
Copy link
Member

Choose a reason for hiding this comment

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

Should we assert this field is properly stored?

Copy link
Member

Choose a reason for hiding this comment

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

Test will be updated at a later stage.

@imanoleguskiza imanoleguskiza merged commit e0b0b60 into EPIC/video-iframe-media-v1 Jun 11, 2020
@imanoleguskiza imanoleguskiza deleted the OPENEUROPA-3295 branch June 11, 2020 14:03
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.

3 participants