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

feat: responsive caption settings #5534

Merged
merged 12 commits into from Nov 14, 2018
Merged

feat: responsive caption settings #5534

merged 12 commits into from Nov 14, 2018

Conversation

@brandonocasey
Copy link
Contributor

@brandonocasey brandonocasey commented Oct 30, 2018

  • Makes reset/done buttons take up two columns and centers them
  • Makes the width of all selects the same
  • When the caption settings are open on a small player, move to a single column layout. Currently there is a horizontal scroll bar.
@brandonocasey brandonocasey force-pushed the feat/responsive-caption-settings branch from 4ebbc9e to e7085df Oct 30, 2018
@@ -375,7 +375,7 @@ class TextTrackSettings extends ModalDialog {
this.localize('Background'),
'</legend>',
this.createElSelect_('backgroundColor', legendId),
'<span class="vjs-bg-opacity vjs-opacity">',
'<span class="vjs-bg-opacity vjs-opacity vjs-track-setting">',
Copy link
Contributor Author

@brandonocasey brandonocasey Oct 30, 2018

added this so that all the selects below them could be the same width.

@brandonocasey brandonocasey force-pushed the feat/responsive-caption-settings branch 2 times, most recently from 0b68c5c to 334b39f Oct 30, 2018
@@ -102,4 +117,5 @@

.vjs-track-settings-controls .vjs-default-button {
margin-right: 1em;
margin-bottom: 1em;
Copy link
Contributor Author

@brandonocasey brandonocasey Oct 30, 2018

the bottom buttons appear right on the border of the dialog box for small players, this fixes that.

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 2, 2018

Hm... the netlify deploy is broken. The CSS isn't loading: https://deploy-preview-5534--videojs-docs.netlify.com/test-example/

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 2, 2018

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 2, 2018

There's a lot of extra whitespace with the changes made here.
screen shot 2018-11-02 at 2 55 21 pm
compared to what it looks like now
screen shot 2018-11-02 at 2 57 18 pm
Though, we can probably make it a bit better by increasing the height of the dialog a bit, so, at this size the buttons aren't cut off.



// 1 column for small players
@media (max-width: 425px) {
Copy link
Member

@gkatsev gkatsev Nov 2, 2018

what about using the responsive classes instead of media queries?
Also, shouldn't grid automatically resize if we set a minimum size?

Copy link
Contributor Author

@brandonocasey brandonocasey Nov 2, 2018

If responsive was the default, I would think that would be a good idea. Going to check on the grid with a minimum size.

Copy link
Member

@gkatsev gkatsev Nov 2, 2018

I think it's reasonable to use that feature, though. The idea of the feature is that components can use it to be able to expect what size the player is without doing extra stuff in javascript themselves. I'd want to have it enabled by default eventually.

But if it can just automatically resize based on grid, that would be even better, probably.

Copy link
Contributor Author

@brandonocasey brandonocasey Nov 6, 2018

Seems like we cannot auto-resize the grid, at least from all I have tried, I changed it to use the responsive class.

@brandonocasey
Copy link
Contributor Author

@brandonocasey brandonocasey commented Nov 2, 2018

Going to play around with this some more to reduce the whitespace.

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 2, 2018

I'd expect it to still look about the same as it does currently.

@brandonocasey brandonocasey force-pushed the feat/responsive-caption-settings branch from fca186e to b97cf7b Nov 6, 2018
@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 6, 2018

The buttons should be in the bottom right corner rather than in the middle of the dialog.

@@ -17,7 +17,7 @@
}

.vjs-text-track-settings .vjs-track-settings-controls {
text-align: right;
text-align: center;
Copy link
Member

@gkatsev gkatsev Nov 6, 2018

this should stay right

src/css/components/_captions-settings.scss Show resolved Hide resolved
@@ -64,7 +58,7 @@

.vjs-text-track-settings legend {
color: $primary-foreground-color;
margin: 0 0 5px 0;
margin: 0 0 1px 0;
Copy link
Member

@gkatsev gkatsev Nov 6, 2018

why make this 1px?

}

.vjs-text-track-settings fieldset {
margin: 5px;
Copy link
Member

@gkatsev gkatsev Nov 6, 2018

why remove this margin and padding?

@misteroneill
Copy link
Member

@misteroneill misteroneill commented Nov 8, 2018

Agree with @gkatsev's input above.

@@ -101,5 +97,5 @@
}

.vjs-track-settings-controls .vjs-default-button {
margin-right: 1em;
margin: 1em;
Copy link
Member

@gkatsev gkatsev Nov 8, 2018

with this change, the controls end up being 20px+1em from the button of the dialog, if we keep it as 1em, they'll only be the 20px from the bottom and look more consistent with the other controls.
The 20px come from the padding of the dialog itself.

Copy link
Contributor Author

@brandonocasey brandonocasey Nov 8, 2018

So the reason why this was changed, was that when the player is small the reset/done buttons touch the very bottom of the captions-setting modal. Maybe we can just add the margin-bottom at small sizes, and make this margin-right again.'

Copy link
Member

@gkatsev gkatsev Nov 8, 2018

Ah, didn't notice that. Looking into it, it seems that when it overflows, the "modal" itself doesn't resize, so, we don't get the padding from it. It looks like what we want to do is set overflow: scroll on the modal itself when we know it'll scroll, or probably just always set overflow scroll for this modal.
Tried it out and seems to work.

Copy link
Contributor Author

@brandonocasey brandonocasey Nov 8, 2018

after testing this out, it worked but only on chrome, on firefox it actually made the settings go off the modal, and they could not be scrolled to.

grid-column: 2;
grid-row: 2;

}
}

// Form elements
.vjs-track-setting > select {
margin-right: 5px;
Copy link
Member

@misteroneill misteroneill Nov 8, 2018

I think we ought to make this a bit bigger. And ems instead - maybe 1em?

Also, I wonder if we can get some vertical spacing for the select controls? One way to do this would be:

  • Add margin-bottom: 1em; here.
  • Remove all margin from .vjs-text-track-settings fieldset.

That looks a lot better at all sizes, I think.

Copy link
Member

@gkatsev gkatsev Nov 8, 2018

Works for me.

Copy link
Contributor Author

@brandonocasey brandonocasey Nov 8, 2018

testing


// halfway through vjs-layout-medium side-by-side selects in
// track-settings-colors collapse, apply fixes as needed
@media (max-width: 563px) {
Copy link
Contributor Author

@brandonocasey brandonocasey Nov 8, 2018

When the selects collapse in vjs-layout-medium the right fieldset for font settings is no longer aligned with the left. This is the only fix I could think of, if we don't think it's an issue we can remove it.

Copy link
Member

@gkatsev gkatsev Nov 8, 2018

I'm not sure this really matters. Each column can layout by itself, plus, 4 lines less CSS :)

Copy link
Contributor Author

@brandonocasey brandonocasey Nov 8, 2018

ok removing

@brandonocasey brandonocasey force-pushed the feat/responsive-caption-settings branch from d936175 to f973cbb Nov 8, 2018
@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 8, 2018

Looks like the caption controls are hitting the bottom of the dialog on scroll in non-responsive mode again. Otherwise, this is looking super good.

@brandonocasey brandonocasey force-pushed the feat/responsive-caption-settings branch from 0ad9459 to 0df2949 Nov 9, 2018
@@ -62,6 +60,11 @@
display: inline-block;
}

// style the second select for text colors
.vjs-text-track-settings fieldset span > select {
max-width: 7.3em;
Copy link
Contributor Author

@brandonocasey brandonocasey Nov 9, 2018

At 7.3em the selects won't ever collapse, so that we can ensure that we always have the right min-height so that the controls never touch the bottom of the modal.

.vjs-layout-x-small .vjs-text-track-settings .vjs-modal-dialog-content,
.vjs-layout-tiny .vjs-text-track-settings .vjs-modal-dialog-content {
grid-template-columns: 1fr;
min-height: 34.2em;
Copy link
Contributor Author

@brandonocasey brandonocasey Nov 9, 2018

We have to update the min-height here so that controls never touch the bottom of the modal

@@ -26,30 +26,28 @@
.vjs-text-track-settings .vjs-modal-dialog-content {
display: grid;
grid-template-columns: 1fr 1fr;
grid-template-rows: 1fr auto;
grid-template-rows: 1fr;
min-height: 23em;
Copy link
Contributor Author

@brandonocasey brandonocasey Nov 9, 2018

We set the min-height here so that the settings-controls will never touch the bottom of the modal.

@brandonocasey
Copy link
Contributor Author

@brandonocasey brandonocasey commented Nov 9, 2018

Should be ready for another review, It took me a long time to get the buttons not to hit the bottom of the modal. It seems like it won't actually be possible to do this for small players in non-responsive mode. I think thats OK though and we should just recommend they use responsive mode.

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 9, 2018

I think thats OK though and we should just recommend they use responsive mode.

This is a regression, we should try not to be introducing regressions even if there are better options for people to use. Let's talk some more next week.

@brandonocasey
Copy link
Contributor Author

@brandonocasey brandonocasey commented Nov 13, 2018

Currently small players (even the default one in sandbox/index.html) sit on the bottom of the modal border and cannot scroll any further:

200 height player in master
screen shot 2018-11-13 at 11 05 22 am

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 13, 2018

Ok, yeah, I can repro. Looks like adding a margin-bottom to vjs-track-settings-controls seems to help. Would that interfere with the rest of the styles? We could have it only be set when responsive mode isn't enabled.

@brandonocasey
Copy link
Contributor Author

@brandonocasey brandonocasey commented Nov 13, 2018

Seems like that won't work. To clarify the current issue. Chrome appears to work at all sizes and in responsive/not responsive mode with the current pull request. Firefox works in responsive mode, but in non-responsive mode at sizes we would consider less than medium lose the bottom gap. I am going to see if there is anything we can do about it

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 13, 2018

Given that it's not a regression, if we don't have a fix for it by EOD, we can just continue without it. However, we should then open a new issue to track this so that we remember to get back to it at some point.

@brandonocasey brandonocasey force-pushed the feat/responsive-caption-settings branch from 963bc52 to b069bf5 Nov 13, 2018
@brandonocasey
Copy link
Contributor Author

@brandonocasey brandonocasey commented Nov 13, 2018

OK I think I got it working, due to spec implementation details in every browser other than chrome flexbox (and grid apparently) layouts can lose their bottom padding. The fix was adding a margin to the bottom of one of the buttons in the last grid row, and removing the bottom padding from the container. The other tricky thing here was that it had to be done within the supports grid context, so that IE did not loose its padding.

@@ -26,30 +26,36 @@
.vjs-text-track-settings .vjs-modal-dialog-content {
display: grid;
grid-template-columns: 1fr 1fr;
grid-template-rows: 1fr auto;
grid-template-rows: 1fr;
// flex an grid for firefox, ie, edge remove bottom padding/margin in a container as size decreases
Copy link
Member

@OwenEdwards OwenEdwards Nov 13, 2018

This comment isn't clear to me - is there a typo in "flex an grid"? Is it "... for firefox, ie & edge" or "... for firefox. ie & edge ..."?

Copy link
Member

@gkatsev gkatsev Nov 13, 2018

yeah, should be flex and grid

Copy link
Member

@gkatsev gkatsev left a comment

LGTM

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 13, 2018

I updated the comment.

@gkatsev gkatsev merged commit b67fe27 into master Nov 14, 2018
4 checks passed
@gkatsev gkatsev deleted the feat/responsive-caption-settings branch Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants