Problem/Motivation

Currently, when swapping the skin to "minimal", not only the skin changes, but also the javascript file.

This isn't needed, as the default.js files also contain the minimal skin. The minimal.js library defintions could stay, but should be commented appropriately.

Steps to reproduce

Proposed resolution

[10.x] Setting "skin" to minimal shouldn't swap the js library

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork videojs-3619649

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

grevil created an issue. See original summary.

grevil opened merge request !34

grevil’s picture

grevil commented
Status: Active » Needs review

I actually removed the minimal js libraries. I think they will just confuse us.

Please review!

grevil’s picture

grevil commented
Status: Needs review » Reviewed & tested by the community

  • grevil committed a85c708e on 10.x 
    fix: #3619649 [10.x] Setting "skin" to minimal shouldn't swap the js...
grevil’s picture

grevil commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Status: Fixed » Active
Issue tags: +Needs discussion

As I have already said, I'm still not sure whether we should remove the library definition entirely.

There might be developers who install the module just to have the library available, and I don't see any reason right now why we shouldn't make the minimal build available. After all, it doesn't involve any extra work for future module or library updates, does it?

I'm only talking about libraries.yml; the rest is fine.

Maybe @anybody has a strong opinion about that?

grevil’s picture

grevil commented

I just don't like defining a libraries.yml entry for a file we don't use at all. If someone wants to use the minimal js file, he can simply alter the video.js library entries. But I am not even sure, if everything the module provides even works with that library.

Of course someone could use the minimal library standalone, via the (now missing) libraries.yml. But I wouldn't know why.

grevil’s picture

grevil commented
Assigned: Unassigned » anybody
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: anybody » Unassigned
Status: Active » Fixed
Issue tags: -Needs discussion

Removing what we don't need is fine. If someone really needs the minimal version this always includes some kind of coding, so they could also add the library themselves.

Less code, less confusion. Setting this FIXED as it's already merged.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.