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

Do not apply new max-width to floating player #3310

Merged
merged 1 commit into from Mar 5, 2019

Conversation

pajong
Copy link
Contributor

@pajong pajong commented Mar 4, 2019

This PR will...

Not apply new max-width to the floating player

Why is this Pull Request needed?

Applying new max-width made the floating player larger on desktop, which we did not want

Are there any points in the code the reviewer needs to double check?

It looks like the code was added for mobile - I did a quick run on mobile and it looked fine removing this, but if there were some specific cases, please let me know

Are there any Pull Requests open in other repos which need to be merged with this?

Addresses Issue(s):

JW8-###

@pajong pajong requested a review from robwalch March 4, 2019 22:33
@johnBartos
Copy link
Contributor

Warnings
⚠️

🛠 There are modified src files, but no test changes. Add tests if you're able to.

⚠️

🗿 Set a milestone. It should be the ticket's fix version in JIRA.

Generated by 🚫 dangerJS

@jwplayer-robot
Copy link

MULTI Build for commit fad98d4 passed.
🏗️ jwplayer build SUCCESS
🏗️ jwplayer browserstack tests SUCCESS
🏗️ jwplayer-commercial build SUCCESS
🏗️ jwplayer-commercial browserstack tests SUCCESS
🥒 Automated Tests SUCCESS
🍆 Manual Tests
📺 Views

@DanFerrer DanFerrer added this to the 8.8.0 milestone Mar 4, 2019
@@ -901,9 +901,7 @@ function View(_api, _model) {
const width = _model.get('width');
const height = _model.get('height');
const styles = getPlayerSizeStyles(width);
if (isNumber(width)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@DanFerrer does this revert some of your aspect ratio work from earlier? are there any considerations for the mobile case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pajong it seems to me like the problem is a missing size constraint. If it appears too big on desktop, i would say we should enforce a specific width or a smaller max-width, instead of removing max-width

Copy link
Contributor

Choose a reason for hiding this comment

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

@jnatalzia This shouldn't break anything I'm aware of. We already have size constraints for max-width in floatingplayer.less at 400pxwith the exception of mobile portrait where we set the max-width to none

@pajong pajong merged commit 9cb1b67 into master Mar 5, 2019
pajong added a commit that referenced this pull request Mar 7, 2019
…g-drag-exclude-ads

* origin/master:
  v8.8.0-beta.1
  Do not apply new max-width to floating player (#3310)
  JW8-5615 Normalize the disabling of floating in the config
  Move _pauseWhenNotViewable() outside of _checkPauseWhenViewable(), addressed Jong's fb
  Changed name of function, addressed Joe's feedback
  Handle VAST ads setting player to idle
@robwalch robwalch deleted the task/fix-floating-sizing branch April 30, 2020 18:29
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

6 participants