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

Volume bar isn't represented as it should #1

Open
ferferga opened this issue May 21, 2019 · 5 comments
Open

Volume bar isn't represented as it should #1

ferferga opened this issue May 21, 2019 · 5 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@ferferga
Copy link
Owner

As you can see in the HTML's mockup of my module at L131, the height of the div container that handles the volume bar is set using a percentage in CSS instead of using a pixel value.

However, while updating the DOM at L238, the bar height is set using pixels value instead of using a percentage as can be seen in L239


While developing the mockup of the module, I had multiple problems with the volume bar positioning and functionality. I originally used a display: grid schema for the whole page, but I had the same problem with Mozilla and Chrome, and height couldn't be set using percentages. Moving to display: flex did the trick in Mozilla, although not in Chrome at the beginning. By removing other properties like vertical-align: bottom, the page worked great using percentages in both browsers. However, with the same exact CSS of the mockup applied in the DOM's of MagicMirror, the percentage values didn't work.

Testing across different Chrome versions, older versions had issues representing the styling that the latest Chrome version represents correctly now. So it seems for me that the only solution for this would be to have an updated version of MagicMirror that has a more modern Electron version with an updated Chrome engine.

PS: The CSS styling that the module now applies in MagicMirror differs slightly from the CSS of the mockup to improve the alignment of the content. However, all the testing was conducted with the exact same CSS of the mockup. Neither passing the this.volume * 100 value or setting a manual value like height: 65% in L239 did the trick. Modifyng the styling using the Developer tools at CTRL + SHIFT + I didn't work either.

@ferferga
Copy link
Owner Author

@MichMich
Copy link

What's the generated style which doesn't work? Can you output the generated style string? It's hard to believe Electron messes up such simple css.

@ferferga
Copy link
Owner Author

ferferga commented May 22, 2019

What's the generated style which doesn't work? Can you output the generated style string? It's hard to believe Electron messes up such simple css.

Hello @MichMich ! I don't understand what do you mean exactly. Do you want me to paste the styling related to the module here as it appears in the mirror's console developer's tool console?

The styling rules used in the module as the same as in the mockup, having some differences only in the margins of some elements. And, as explained in first post, the exact same style rules of the mockup, without any changes, was tested in the module during development.

@MichMich
Copy link

Line 238 generates an inline style based on the volume. This style doesn't work fo you. Could you give me an example of a generated inline style that isn't correctly rendered?

ferferga added a commit that referenced this issue May 31, 2019
@ferferga
Copy link
Owner Author

ferferga commented May 31, 2019

First, I apologise for taking so long to answer.

Secondly, here you have some examples taen from the Developer Tools of Electron:

background-color: white; border: 4px solid white; vertical-align: bottom; border-radius: 5px; height: 80%;
background-color: white; border: 4px solid white; vertical-align: bottom; border-radius: 5px; height: 90%;
background-color: white; border: 4px solid white; vertical-align: bottom; border-radius: 5px; height: 40%;

The percentage it's the same than the volume level of the Google Cast device.
I've made a branch with the original code that should work

@ferferga ferferga assigned ferferga and unassigned ferferga May 31, 2019
@ferferga ferferga pinned this issue May 31, 2019
@ferferga ferferga added bug Something isn't working help wanted Extra attention is needed labels Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants