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-2416: Allow to define which view modes are embedable. #79

Merged
merged 30 commits into from
Feb 20, 2020

Conversation

imanoleguskiza
Copy link
Member

OPENEUROPA-2416

Description

Add functionality to define whether a media view display can be embeded or not.

Change log

  • Added: Add functionality to define whether a media view display can be embeded or not.
  • Changed:
  • Deprecated:
  • Removed:
  • Fixed:
  • Security:

Commands

[Insert commands here]

Copy link
Member

@sergepavle sergepavle left a comment

Choose a reason for hiding this comment

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

I think here missed upgrade path, because, otherwise after release, possibility to embed media will be blocked.
I also have added 1 minor remark.

@imanoleguskiza imanoleguskiza force-pushed the OPENEUROPA-2416 branch 4 times, most recently from b44063f to 22334ad Compare January 7, 2020 15:50
sergepavle
sergepavle previously approved these changes Jan 8, 2020
Copy link
Contributor

@upchuk upchuk left a comment

Choose a reason for hiding this comment

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

  • Let's fix the new Embed view modes to have sensible defaults also in the component in terms of what fields we render. Only for newly installed sites, not also in the upgrade path. For example, the video should only show the video oEmbed and not also the created and authored fields etc.
  • You have a recurring typo: embedable

$this->assertSession()->pageTextContainsOnce('Selected entity');
$this->assertSession()->linkExists('Digital Single Market: cheaper calls to other EU countries as of 15 May');
$this->assertSession()->pageTextContainsOnce('There is no embedable view mode for this media type.');
$this->assertSession()->buttonNotExists("Embed");
Copy link
Contributor

Choose a reason for hiding this comment

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

Single quotes

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 needs to be done still.

upchuk
upchuk previously approved these changes Jan 14, 2020
Copy link
Contributor

@brummbar brummbar left a comment

Choose a reason for hiding this comment

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

Let's also improve the README of the submodule. At the moment to use this component there is a lot to do: create a text format, enable the editor on it, add the button, enable the filter and place it last (very important), allow the embeddable displays (for users). Let's add all of this in there.

* @return array
* The form element.
*/
protected function getMediaViewModeFormElement(FormStateInterface $form_state) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add the needed parameters to this method, so $entity_element and $entity, as the form state is not really needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

We also need the embed_button from the form_state. Does that change anything? :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Where? I cannot see it :(

Copy link
Contributor

@brummbar brummbar left a comment

Choose a reason for hiding this comment

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

After last week meetings with the DIGIT developers, I unfortunately have to ask more changes in here.
Since now we allow to embed only the enabled view modes, existing websites will get into a state that nothing is embeddable. This was the reason that we were shipping a new embed view mode so that could embed something, but this won't be matching their current view modes and relative configuration. So we will:

  • ship the embed view mode only for new installations. We remove the update path creation from here. All the embed view modes must be in the optional config folder.
  • add an update path that allow to embed all the existing view modes (was "default" embeddable before? If not, don't add it). This will prevent any disruption for existing websites.

@@ -2,7 +2,10 @@

The OpenEuropa Media Embed module allows the embedding of Media entities into content in an agnostic (non-Drupal) way.

To this end, it comes with two main elements: the WYSIWYG embed button and the filter plugin.
To this end, it comes with two main elements: the WYSIWYG embed button and the filter plugin. On top of that, it forces
Copy link
Contributor

Choose a reason for hiding this comment

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

It allows site administrators, while it's true that if you don't pick any you cannot embed, "forces" sounds bad.


## Embeddable view modes

Site adminstrators will need to define which available view modes are also available to be embedded via the tools described above.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Site adminstrators will need to define which available view modes are also available to be embedded via the tools described above.
Site administrators will need to define which available view modes are also available to be embedded via the tools described above.

## Embeddable view modes

Site adminstrators will need to define which available view modes are also available to be embedded via the tools described above.
This is done by selecting the available view modes on the display mode configuration page for each available entity bundle.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This is done by selecting the available view modes on the display mode configuration page for each available entity bundle.
This is done by selecting the available view modes on the display mode configuration page for each available media entity bundle.

or

Suggested change
This is done by selecting the available view modes on the display mode configuration page for each available entity bundle.
This is done by selecting the available view modes on the display mode configuration page for each available media bundle.

In order to use the functionalities of the module, first you will need to create a text format.
You can find more information on how to do that on the official [documentation][2]

Once that is done you have to make sure that:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use a step (ordered/unordered list) by step procedure here.

  1. Create a text format can stay there where you put it;
  2. Enable the CKEditor on it;
  3. Add the media button to the toolbar;
  4. Enable the text filter and place it last (very important as you wrote - perfect)
  5. Go to the media display page (not content type) (provide example link) and enable which view modes. The embed view mode is shipped and enabled by default (is it?)

$this->assertSession()->pageTextContainsOnce('Selected entity');
$this->assertSession()->linkExists('Digital Single Market: cheaper calls to other EU countries as of 15 May');
$this->assertSession()->pageTextContainsOnce('There is no embedable view mode for this media type.');
$this->assertSession()->buttonNotExists("Embed");
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 needs to be done still.

@imanoleguskiza imanoleguskiza force-pushed the OPENEUROPA-2416 branch 2 times, most recently from bfc971e to 9b36942 Compare January 23, 2020 11:21
@upchuk upchuk force-pushed the OPENEUROPA-2416 branch 2 times, most recently from 2fff0ec to 717446c Compare January 27, 2020 07:51
upchuk
upchuk previously approved these changes Jan 27, 2020
@brummbar brummbar force-pushed the OPENEUROPA-2416 branch 2 times, most recently from 5721f45 to 37ca2fe Compare January 28, 2020 14:01
22Alexandra
22Alexandra previously approved these changes Feb 18, 2020
@brummbar brummbar merged commit 324f738 into master Feb 20, 2020
@brummbar brummbar deleted the OPENEUROPA-2416 branch February 20, 2020 12:11
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