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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

display placeholder for video-widget #3885

Merged
merged 7 commits into from Sep 29, 2022

Conversation

ETLaurent
Copy link
Contributor

@ETLaurent ETLaurent commented Sep 20, 2022

Depends on #3882

Summary

Display a default placeholder image when adding a video widget.

What are the specific steps to test this change?

On testbed:

  • add a video widget
    馃憠 the initial modal should not appear
    馃憠 the default placeholder video should have been added:

image

馃憠 when you actually set a video for that widget, it should replace the placeholder

  • override the placeholderUrl option in image widget:
// in testbed's "modules/@apostrophecms/video-widget/index.js"
options: {
    placeholderUrl: 'https://vimeo.com/57946935'
  }

馃憠 you should have your video as placeholder now, instead of the default one

馃憠 all of these tests should be effective inside another widget such as the two-column-widget (nesting)
馃憠 all of these tests should be effective in the page editor modal

What kind of change does this PR introduce?

(Check at least one)

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Build-related changes
  • Other

Make sure the PR fulfills these requirements:

  • It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
  • The changelog is updated
  • Related documentation has been updated
  • Related tests have been updated

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

@linear
Copy link

linear bot commented Sep 20, 2022

PRO-3145 Implement standard placeholder for video widgets, option to override

  • When aposPlaceholder: true is set on the widget, video widgets should display a standard static image asset (yes an image, not a video) by default per Stu's design until the user edits the widget (cc @stuart to get this asset).
  • This should be implemented via a special case in widget.html.
  • Sitewide override should be supported via the placeholderUrl module-level option, which should point to the default asset initially.
  • The default asset should ship in the public subdir of the module.
  • The asset URL should be resolved using apos.asset.url(getOption('placeholderUrl')) in widget.html so that it still resolves properly if the public assets have been deployed to S3 etc.

Same person who does this should also do PRO-3144 .

Acceptance criteria

  • Testbed project contains an example of this.

@ETLaurent ETLaurent marked this pull request as ready for review September 20, 2022 14:56
@ETLaurent
Copy link
Contributor Author

馃И Unit tests will be added after #3886 is merged.

<img
src="{{ apos.asset.url(data.manager.options.placeholderUrl) }}"
alt="image placeholder"
width="100%"
Copy link
Member

Choose a reason for hiding this comment

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

Same concern. Use a class, provide default styles that can be overridden.

Copy link
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

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

Base automatically changed from pro-3131-placeholder-wrapper to feature-widget-placeholders September 28, 2022 13:03
@ETLaurent ETLaurent merged commit c71a8b4 into feature-widget-placeholders Sep 29, 2022
@ETLaurent ETLaurent deleted the pro-3145-placeholder-video branch September 29, 2022 10:55
ETLaurent added a commit that referenced this pull request Oct 11, 2022
ETLaurent added a commit that referenced this pull request Oct 24, 2022
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.

None yet

2 participants