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

Added toggable button for explanation #35

Merged
merged 3 commits into from Oct 8, 2019
Merged

Conversation

@agarcia-caicedo
Copy link
Contributor

agarcia-caicedo commented Oct 4, 2019

I added a button that when clicked will show the user an explanation of the difference between viewport size and resolution

@coogie coogie self-requested a review Oct 5, 2019
@coogie coogie self-assigned this Oct 5, 2019
Copy link
Owner

coogie left a comment

Thanks for the PR, @agarcia-caicedo! I appreciate the work you've put into it.

I have some feedback that I've requested in the files. If you have any questions, just let me know!

font-size: 12pt;
}

#Toggle-Explanation{

This comment has been minimized.

Copy link
@coogie

coogie Oct 5, 2019

Owner

Can you use a class name to apply styles, instead of an ID?

@@ -170,6 +170,28 @@ body {
text-decoration: none;
}

#Explanation{

This comment has been minimized.

Copy link
@coogie

coogie Oct 5, 2019

Owner

Can you use a class name to apply styles, instead of an ID?

@@ -170,6 +170,28 @@ body {
text-decoration: none;
}

#Explanation{
font-size: 12pt;

This comment has been minimized.

Copy link
@coogie

coogie Oct 5, 2019

Owner

You can use rem instead of pt

Suggested change
font-size: 12pt;
font-size: 1rem;
}

#Toggle-Explanation{
background: #f1652991;

This comment has been minimized.

Copy link
@coogie

coogie Oct 5, 2019

Owner

Unfortunately, RGBA hex is not supported in IE and Edge

Instead, can you use the existing CSS Custom Properties for this, with the Sass variable as the fallback (must come first). This would ensure that this looks normal in dark mode. See below:

Suggested change
background: #f1652991;
background-color: $theme-primary;
background-color: var(--theme-primary);

#Toggle-Explanation{
background: #f1652991;
font-size: 12pt;

This comment has been minimized.

Copy link
@coogie

coogie Oct 5, 2019

Owner

You can use rem instead of pt

Suggested change
font-size: 12pt;
font-size: 1rem;
@@ -5,10 +5,19 @@ function displayViewport(event) {
document.getElementById('h').innerHTML = verge.viewportH();
}

function explanation() {
var x = document.getElementById("Explination");

This comment has been minimized.

Copy link
@coogie

coogie Oct 5, 2019

Owner
Suggested change
var x = document.getElementById("Explination");
var x = document.getElementById("Explanation");
function explanation() {
var x = document.getElementById("Explination");
if (x.innerHTML == "") {
x.innerHTML = "Screen resolution is not the same as viewport size. Viewport refers to the visible area of a webpage on a device, while Screen resolution is the amount of pixels a device has";

This comment has been minimized.

Copy link
@coogie

coogie Oct 5, 2019

Owner

Instead of adding and removing content to the DOM with JS, can you instead just put this text inside the div and toggle its display value between block/none?

background: #f1652991;
font-size: 12pt;
border: none;
color: white ;

This comment has been minimized.

Copy link
@coogie

coogie Oct 5, 2019

Owner

Similar here, can you use the CSS Custom Property with a fallback?

Suggested change
color: white ;
color: #fff;
color: var(--theme-color);
index.html Outdated
@@ -38,6 +38,9 @@ <h1 class="App__title">Your viewport size is:</h1>
&times;
<span id="h"></span>
</p>

<button onclick="explanation()" id="Toggle-Explanation">Is this my screen resolution?</button>
<div id="Explanation"></div>

This comment has been minimized.

Copy link
@coogie

coogie Oct 5, 2019

Owner

Can this be a p tag instead? :)

@@ -170,6 +170,28 @@ body {
text-decoration: none;

This comment has been minimized.

Copy link
@coogie

coogie Oct 5, 2019

Owner

Can this file have its extension restored to .scss?
This is a Sass file which eventually compiles into CSS. If it is a .css file, the compilation doesn't happen to it and the styles break.

@coogie coogie added the enhancement label Oct 5, 2019
@agarcia-caicedo

This comment has been minimized.

Copy link
Contributor Author

agarcia-caicedo commented Oct 8, 2019

I have made the updates you have suggested to the code. I think I have made all the changes, but I'll be glad to make any additional ones if they are required.

ae63f20

@coogie
coogie approved these changes Oct 8, 2019
@coogie coogie changed the title Fixed Issue #22 Added toggable button for explination Added toggable button for explanation Oct 8, 2019
@coogie coogie merged commit ee3112b into coogie:master Oct 8, 2019
@coogie

This comment has been minimized.

Copy link
Owner

coogie commented Oct 8, 2019

Thanks for making the changes and all the effort you put into this! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.