-
Notifications
You must be signed in to change notification settings - Fork 6
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 fixed navbar #46
base: master
Are you sure you want to change the base?
Conversation
source-browser.html
Outdated
@@ -42,6 +69,16 @@ | |||
</style> | |||
</head> | |||
<body class="m2"> | |||
<ul class = "navbar" id="navbar"> | |||
<li><a href="#languages" id="lilang">languages</a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should not be hardcoded. They should be generated by the JS.
source-browser.html
Outdated
@@ -72,16 +109,16 @@ <h1 id="loading" class="m1 center display-none">⌛</h1> | |||
name: topic, | |||
repoUrl: `https://github.com/${ORGANIZATION}/apertium-${topic}`, | |||
})); | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove all these extra trailing spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not fixed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed these from throughout the code.
source-browser.html
Outdated
@@ -132,7 +169,7 @@ <h1 id="loading" class="m1 center display-none">⌛</h1> | |||
$('#topics').append( | |||
$('<div class="topic flex-auto">').append( | |||
$('<div>').append( | |||
$('<h2 class="inline-block m0">').text(name), | |||
$('<h2 class="inline-block m0" id='+name+'>').text(name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use concatenation. Use a jQuery function to set the id
.
source-browser.html
Outdated
$(window).scroll(function() { | ||
var top = $(window).scrollTop(); | ||
var bottom = $(window).scrollTop() + $(window).height(); | ||
var langtop = $("#languages").position(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be adjusted via a loop that goes through the topics, not hardcoded to only work with these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial comments. You might find it easiest to run this code through Prettier with the correct settings.
source-browser.html
Outdated
@@ -42,6 +65,7 @@ | |||
</style> | |||
</head> | |||
<body class="m2"> | |||
<ul class = "navbar" id="navbar"></ul> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove spacing around first equal sign.
source-browser.html
Outdated
@@ -72,16 +109,16 @@ <h1 id="loading" class="m1 center display-none">⌛</h1> | |||
name: topic, | |||
repoUrl: `https://github.com/${ORGANIZATION}/apertium-${topic}`, | |||
})); | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not fixed...
source-browser.html
Outdated
|
||
|
||
|
||
var array = ['languages', 'trunk', 'staging', 'nursery', 'incubator', 'tools']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already defined above as TOPICS.
source-browser.html
Outdated
|
||
var array = ['languages', 'trunk', 'staging', 'nursery', 'incubator', 'tools']; | ||
array.forEach(function(header) { | ||
$("#navbar").append("<li><a href=#"+header+" id=li"+header+">"+header+"</a></li>"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be no concatenation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try using the href and text methods. jQuery elements can also take a list of attributes. Nested elements should be constructed via append for readability.
source-browser.html
Outdated
var array = ['languages', 'trunk', 'staging', 'nursery', 'incubator', 'tools']; | ||
array.forEach(function(header) { | ||
$("#navbar").append("<li><a href=#"+header+" id=li"+header+">"+header+"</a></li>"); | ||
$("#inline-block m0").attr("id", name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What element is this adding to? Why is there an element with this misleading id that sounds like a class?
source-browser.html
Outdated
$("#inline-block m0").attr("id", name); | ||
}); | ||
$(window).scroll(function() { | ||
var top = $(window).scrollTop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use only const or let, never var.
source-browser.html
Outdated
var bottom = $(window).scrollTop() + $(window).height(); | ||
var titles = ['languages', 'trunk', 'staging', 'nursery', 'incubator', 'tools']; | ||
var i; | ||
for(i = 0; i<titles.length; i++){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need spacing between binary operators and i should be declared inline here.
source-browser.html
Outdated
$(window).scroll(function() { | ||
var top = $(window).scrollTop(); | ||
var bottom = $(window).scrollTop() + $(window).height(); | ||
var titles = ['languages', 'trunk', 'staging', 'nursery', 'incubator', 'tools']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this declared again?
source-browser.html
Outdated
for(i = 0; i<titles.length; i++){ | ||
var header = titles[i]; | ||
var vartop = $('#'+header).position(); | ||
if(top < vartop.top && vartop.top < bottom){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please emulate spacing in the rest of this code.
source-browser.html
Outdated
if(vartop.top>bottom){ | ||
$tools.text(header+'↓') | ||
} | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always use braces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some further suggestions. There are a number of old ones that have not been actioned on either.
source-browser.html
Outdated
@@ -67,21 +91,21 @@ <h1 id="loading" class="m1 center display-none">⌛</h1> | |||
CACHE_KEY = `${ORGANIZATION}-cache`, | |||
EXPIRY_KEY = `${ORGANIZATION}-expiry`, | |||
EXPIRY_MILLIS = 60000, | |||
TOPICS = ['languages', 'trunk', 'staging', 'nursery', 'incubator', 'tools'].map(topic => ({ | |||
LIST = ['languages', 'trunk', 'staging', 'nursery', 'incubator', 'tools'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use entirely unhelpful variables names like list
. In fact, this change isn't necessary at all. Just use the previous TOPICS
variable for iteration and access the name
attribute. e.g. TOPICS[0].name
.
source-browser.html
Outdated
async function getReposByTopic(organization) { | ||
const headers = new Headers({ Accept: 'application/vnd.github.mercy-preview+json' }); | ||
const cacheStale = !(CACHE_KEY in localStorage) || localStorage[EXPIRY_KEY] < Date.now(); | ||
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still unnecessary whitespace additions throughout this entire file that should be deleted. Something like 10 of them.
source-browser.html
Outdated
@@ -132,7 +156,7 @@ <h1 id="loading" class="m1 center display-none">⌛</h1> | |||
$('#topics').append( | |||
$('<div class="topic flex-auto">').append( | |||
$('<div>').append( | |||
$('<h2 class="inline-block m0">').text(name), | |||
$('<h2 class="inline-block m0" id="topic" style="padding-top:75px; margin-top:-75px;">').text(name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use inline styling. Instead, style the associated class. Furthermore, this creates multiple elements with the same id, i.e. topic
. This is illegal HTML. Instead, something like topic-languages
etc. might work for your purposes.
source-browser.html
Outdated
@@ -141,7 +165,8 @@ <h1 id="loading" class="m1 center display-none">⌛</h1> | |||
repoList, | |||
), | |||
); | |||
|
|||
$("[id=topic]").attr("id", name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment.
source-browser.html
Outdated
else{ | ||
$('#li'+heading).show(); | ||
let $heading = $('#li'+heading); | ||
if(vartop.top>bottom){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spacing between binary operations here and throughout should be consistent with existing code.
source-browser.html
Outdated
$(document).ready(() => { | ||
$("#navbar").append('<li id="li"></li>'); | ||
for(let i = 0; i < LIST.length; i++){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of stylistic consistency, use forEach
here. It might even be nicer with a map
to create the elements and then appending the entire list to the container.
source-browser.html
Outdated
$("#navbar").append('<li id="li"></li>'); | ||
for(let i = 0; i < LIST.length; i++){ | ||
$("#li").append('<a id="placeholder"></a>'); | ||
$("[id=placeholder]").attr({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an anti-pattern. Use the way that multiple attributes are added to an element throughout the existing code, i.e. an object containing the attributes is passed as the second argument to $(...)
when creating the element.
source-browser.html
Outdated
setInterval(refreshTopics, EXPIRY_MILLIS * 2); | ||
showTopics(); | ||
}); | ||
</script> | ||
</body> | ||
</html> | ||
</html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert your changes that deleted the final blank line in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a number of still unresolved comments (at least 3).
source-browser.html
Outdated
padding: 14px 16px; | ||
text-decoration: none; | ||
} | ||
h2[class$="inline-block m0"] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What..?
source-browser.html
Outdated
@@ -72,16 +100,13 @@ <h1 id="loading" class="m1 center display-none">⌛</h1> | |||
name: topic, | |||
repoUrl: `https://github.com/${ORGANIZATION}/apertium-${topic}`, | |||
})); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this spacing change and the others.
source-browser.html
Outdated
padding: 14px 16px; | ||
text-decoration: none; | ||
} | ||
h2[class="inline-block m0"] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look into how to use CSS selectors, especially selecting using classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll take a look at that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed the CSS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix the spacing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still numerous style issues. I've pointed out some of them. As I mentioned, running this through prettier is probably the best option.
source-browser.html
Outdated
function createNavbar(item) { | ||
$('#li').append( | ||
$('<a>', { | ||
id: 'li' + item, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use string interpolation instead of concatenation i.e.
`li${item}`
Also, id
s should be id-agnostic if possible. So, something like ${item}-nav-link
perhaps?
source-browser.html
Outdated
@@ -181,13 +209,51 @@ <h1 id="loading" class="m1 center display-none">⌛</h1> | |||
.focus(); | |||
} | |||
|
|||
function createNavbar(item) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function createNavbar(item) { | |
function createNavbar(topic) { |
source-browser.html
Outdated
for(let i = 0; i < TOPICS.length; i++){ | ||
let heading = TOPICS[i].name; | ||
list.push(heading); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is off here.
source-browser.html
Outdated
$(document).ready(() => { | ||
let list = []; | ||
$("#navbar").append('<li id="li"></li>'); | ||
for(let i = 0; i < TOPICS.length; i++){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a forEach
instead for consistency.
source-browser.html
Outdated
$('#github-link').attr('href', `https://github.com/${ORGANIZATION}`); | ||
$('#refresh').click(({ currentTarget }) => { | ||
currentTarget.blur(); | ||
refreshTopics(); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert spacing change here.
source-browser.html
Outdated
let heading = TOPICS[i].name; | ||
let vartop = $('#' + heading).position(); | ||
if(top < vartop.top && vartop.top < bottom){ | ||
$('#li' + heading).hide(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use string interpolation concatenation.
source-browser.html
Outdated
for(let i = 0; i < TOPICS.length; i++){ | ||
let heading = TOPICS[i].name; | ||
let vartop = $('#' + heading).position(); | ||
if(top < vartop.top && vartop.top < bottom){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(top < vartop.top && vartop.top < bottom){ | |
if (top < vartop.top && vartop.top < bottom) { |
source-browser.html
Outdated
$(window).scroll(() => { | ||
let top = $(window).scrollTop(); | ||
let bottom = $(window).scrollTop() + $(window).height(); | ||
for(let i = 0; i < TOPICS.length; i++){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be possible with forEach
. Don't create a new function, just use an anonymous arrow function.
source-browser.html
Outdated
if(top < vartop.top && vartop.top < bottom){ | ||
$('#li' + heading).hide(); | ||
} | ||
else{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spacing and indentation
source-browser.html
Outdated
else{ | ||
$('#li' + heading).show(); | ||
let $heading = $('#li' + heading); | ||
if(vartop.top > bottom){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(vartop.top > bottom){ | |
if (vartop.top > bottom) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting close!
source-browser.html
Outdated
padding: 14px 16px; | ||
text-decoration: none; | ||
} | ||
.inline-block.m0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be styling all of them with this. Instead, should use a custom class in the JS and style that.
source-browser.html
Outdated
ul:nth-child(1) { | ||
list-style-type: none; | ||
position: absolute; | ||
margin: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many of these rules should be replaced with the basscss v7 rules. e.g. .m0
is margin: 0
.
source-browser.html
Outdated
@@ -42,6 +69,7 @@ | |||
</style> | |||
</head> | |||
<body class="m2"> | |||
<ul class="navbar" id="navbar"></ul> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the id
should suffice unless you have multiple navbar
s.
source-browser.html
Outdated
@@ -181,7 +209,46 @@ <h1 id="loading" class="m1 center display-none">⌛</h1> | |||
.focus(); | |||
} | |||
|
|||
function createNavbar(topic) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inline this function where it is used.
source-browser.html
Outdated
function createNavbar(topic) { | ||
$('.li').append( | ||
$('<a>', { | ||
id: `li${topic}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in my previous review, use a tag-agnostic id.
source-browser.html
Outdated
}); | ||
}); | ||
|
||
$(window).scroll(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also happen on window resize?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added that, as well.
source-browser.html
Outdated
let top = $(window).scrollTop(); | ||
let bottom = $(window).scrollTop() + $(window).height(); | ||
list.forEach(heading => { | ||
let vartop = $(`#${heading}`).position(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the var
in vartop
refer to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It refers to variable, for each of the heading variables. Would you suggest I rename it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to name a variable var
to indicate that it's a variable? Something like heading_position
maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll rename that.
source-browser.html
Outdated
}); | ||
|
||
$(window).scroll(() => { | ||
let top = $(window).scrollTop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variables that do not change should be const
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll update that tomorrow.
source-browser.html
Outdated
$(`#li${heading}`).hide(); | ||
} else { | ||
$(`#li${heading}`).show(); | ||
let $heading = $(`#li${heading}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let $heading = $(`#li${heading}`); | |
$(`#li${heading}`).text(`${heading} ${vartop.top > bottom ? '↓' : '↑'}`); |
source-browser.html
Outdated
$(document).ready(() => { | ||
$('#navbar').append('<li class="li"></li>'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there an extra list element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's only one list element. Would you like me to make this list element hard-coded in the html instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I'll do that. :)
I'm using my dad's computer. That's why the name is different. |
source-browser.html
Outdated
} | ||
.topic { | ||
padding-top:75px; | ||
margin-top:-75px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spacing between colon and value.
source-browser.html
Outdated
@@ -42,6 +50,9 @@ | |||
</style> | |||
</head> | |||
<body class="m2"> | |||
<ul class="fixed m0 p0 overflow-hidden bg-black bottom-0 col-12 z2" id="navbar"> | |||
<li class="li left"></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this li class do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the hardcoded li class that I added. The goes within the <li>, which goes within the <ul>.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used for the .append() method later on in the code, to select only those values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just #navbar li
?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unresolved?
source-browser.html
Outdated
const top = $(window).scrollTop(); | ||
const bottom = $(window).scrollTop() + $(window).height(); | ||
TOPICS.forEach(heading => { | ||
heading = heading.name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use destructuring in the function argument.
source-browser.html
Outdated
const bottom = $(window).scrollTop() + $(window).height(); | ||
TOPICS.forEach(heading => { | ||
heading = heading.name; | ||
let heading_position = $(`#${heading}`).position(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Const
source-browser.html
Outdated
TOPICS.forEach(heading => { | ||
heading = heading.name; | ||
let heading_position = $(`#${heading}`).position(); | ||
if (top < heading_position.top && heading_position.top < bottom) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're only ever using top, just save that instead.
source-browser.html
Outdated
@@ -182,6 +193,32 @@ <h1 id="loading" class="m1 center display-none">⌛</h1> | |||
} | |||
|
|||
$(document).ready(() => { | |||
$(window).on('scroll resize', function(){ | |||
const top = $(window).scrollTop(); | |||
const bottom = $(window).scrollTop() + $(window).height(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use top here instead of another call.
source-browser.html
Outdated
$(`#${heading}-nav-link`).hide(); | ||
} else { | ||
$(`#${heading}-nav-link`).show(); | ||
$(`#${heading}-nav-link`).text(`${heading} ${heading_position.top > bottom ? '↓' : '↑'}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These calls can be chained.
source-browser.html
Outdated
}); | ||
|
||
TOPICS.forEach(topic => { | ||
topic = topic.name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Destructuring here as well.
source-browser.html
Outdated
topic = topic.name; | ||
$('.li').append( | ||
$('<a>', { | ||
class: `inline-block white center btn:hover`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just add this to the string part since there's no interpolation.
source-browser.html
Outdated
$(window).on('scroll resize', function(){ | ||
const top = $(window).scrollTop(); | ||
const bottom = $(window).scrollTop() + $(window).height(); | ||
TOPICS.forEach(heading => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think append can take a list, right? In that case, use a map to create the list and append that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which append are you referring to?
$('.li').append(
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand, that area of the code doesn't need appending.
source-browser.html
Outdated
$(window).on('scroll resize', function(){ | ||
const top = $(window).scrollTop(); | ||
const bottom = top + $(window).height(); | ||
TOPICS.forEach(topic => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TOPICS.forEach(topic => { | |
TOPICS.forEach(({name}) => { |
source-browser.html
Outdated
const bottom = top + $(window).height(); | ||
TOPICS.forEach(topic => { | ||
let {name} = topic; | ||
const name_position = $(`#${name}`).position().top; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const name_position = $(`#${name}`).position().top; | |
const navLinkPosition = $(`#${name}`).position().top; |
source-browser.html
Outdated
@@ -182,6 +193,29 @@ <h1 id="loading" class="m1 center display-none">⌛</h1> | |||
} | |||
|
|||
$(document).ready(() => { | |||
$(window).on('scroll resize', function(){ | |||
const top = $(window).scrollTop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of the code is indented two spaces but now it's 4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll run it through Prettier again, then.
source-browser.html
Outdated
@@ -182,6 +193,29 @@ <h1 id="loading" class="m1 center display-none">⌛</h1> | |||
} | |||
|
|||
$(document).ready(() => { | |||
$(window).on('scroll resize', function(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$(window).on('scroll resize', function(){ | |
$(window).on('scroll resize', () => { |
source-browser.html
Outdated
if (top < name_position && name_position < bottom) { | ||
$(`#${name}-nav-link`).hide(); | ||
} else { | ||
$(`#${name}-nav-link`).text(`${name} ${name_position > bottom ? '↓' : '↑'}`).show(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can replace the entire block with something like:
$(`#${name}-nav-link`).text(`${name} ${name_position > bottom ? '↓' : '↑'}`).show(); | |
$(`#${name}-nav-link`) | |
.text(`${name} ${name_position > bottom ? '↓' : '↑'}`) | |
.toggle(top >= name_position || name_position >= bottom); |
source-browser.html
Outdated
} | ||
}); | ||
}); | ||
TOPICS.forEach(topic => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire block can be replaced with $('.li').append(TOPICS.map(({name}) => ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why are you appending all the links to the same list element? There should be one list element (<li>
) per entry inside the <ul>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried replacing TOPICS.forEach(topic => { as you recommended, but it didn't work.
I also tried to make one list element (each containing one / element), but it didn't quite work. I tried doing this:
TOPICS.map(({name}) => { $('#navbar').append( $('<li></li>', {class:
${name} left}) $('#navbar li').append( $('<a class = "inline-block white center btn:hover">', { id:
${name}-nav-link, href:
#${name} }).text(name) ); ); });
, which failed. Do you have any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, it works now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there!
source-browser.html
Outdated
|
||
TOPICS.map(({name}) => { | ||
$('#navbar').append( | ||
$('<li class = "left">').append( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$('<li class = "left">').append( | |
$('<li class="left">').append( |
source-browser.html
Outdated
}); | ||
|
||
TOPICS.map(({name}) => { | ||
$('#navbar').append( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I suggested using map
, I didn't mean directly instead of forEach
. You should map
to create the <li>
elements and then append that list to #navbar
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So something like this?
$('#navbar').append(
TOPICS.map(({ name }) => {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The js syntax seems to not work when I do that, however. Do you have any suggestions on how to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work. What is the error and where is it occurring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that's interesting:
$('#navbar').append(TOPICS.map(({name}) => {
$('<li class="left">').append(
$('<a class="inline-block white center btn:hover">', {
id:
${name}-nav-link,
href:
#${name},
}).text(name)
)
});
);
That is the code that I edited.
The error message was "Uncaught SyntaxError: missing ) after argument list," which is strange, because I'm pretty sure every bracket and parentheses is closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what the error might be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't have multiple semicolons in one statement. That is, your first semicolon should probably be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I edited the jquery as you suggested, and now the code looks like this:
It still doesn't work, though...I'm pretty sure the syntax is correct, though. Every parentheses/bracket is balanced and closed.
The error message is still the same, however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please just paste code here using the code formatting (4 spaces).
I don't understand why you have two lines that are almost identical? Also, your arrow function needs a return
if you use braces. If you omit them (which is fine here), you can also omit the return
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I've fixed it.
source-browser.html
Outdated
TOPICS.map(({name}) => { | ||
$('#navbar').append( | ||
$('<li class = "left">').append( | ||
$('<a class = "inline-block white center btn:hover">', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$('<a class = "inline-block white center btn:hover">', { | |
$('<a class="inline-block white center btn:hover">', { |
source-browser.html
Outdated
$('<a class = "inline-block white center btn:hover">', { | ||
id: `${name}-nav-link`, | ||
href: `#${name}`, | ||
class: `inline-block white center btn:hover` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't needed anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, I'm not quite sure if I understand. Why don't I need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already have it above in the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On 200? It doesn't define the id, href, or class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not saying the id and href aren't needed. This comment is on the line with the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I'm sorry, I misunderstood what you meant.
source-browser.html
Outdated
$('#github-link').attr('href', `https://github.com/${ORGANIZATION}`); | ||
$('#refresh').click(({ currentTarget }) => { | ||
$('#refresh').click(({currentTarget}) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use the proper prettifier settings to revert this change and also use the same spacing elsewhere?
source-browser.html
Outdated
@@ -42,6 +50,8 @@ | |||
</style> | |||
</head> | |||
<body class="m2"> | |||
<ul class="fixed m0 p0 overflow-hidden bg-black bottom-0 col-12 z2" id="navbar"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<ul class="fixed m0 p0 overflow-hidden bg-black bottom-0 col-12 z2" id="navbar"> | |
<ul id="navbar" class="fixed m0 p0 overflow-hidden bg-black bottom-0 col-12 z2"></ul> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor concerns when I tried it out:
- When I load the page, all 5 navbar entries are visible despite all the headings also being visible.
- The navbar starts strangely offset from the left?
- Clicking the navbar link should go above the header. This is possible with some CSS tricks I think.
- The navbar covers the refresh button.
- The empty navbar still takes up a bunch of space? Was this intentional?
source-browser.html
Outdated
}).text(name) | ||
), | ||
), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spacing is very suspicious here. Try running this through Prettier.
The navbar is 2 pixels offset from the left, because |
Nevermind, I got it to work! :) |
I fixed everything, but there's something wrong about the anchor links: any header that's on the top of the page doesn't work, for some reason. Do you have any ideas about this? I fixed everything else, though. |
Currently, I'm using
which works for all of the other links besides the ones on the top. I also tried using some of the solutions from here: https://css-tricks.com/hash-tag-links-padding/ but it didn't quite work... |
I think that there might be something wrong with the header (with the apertium & github logos), which is making the text headers below messed up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some extra comments and there's a comment from before re. chaining that hasn't been resolved.
source-browser.html
Outdated
#content { | ||
margin: 1rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spacing?
source-browser.html
Outdated
const navLinkPosition = $(`#${name}`).position().top; | ||
$(`#${name}-nav-link`) | ||
.text(`${name} ${navLinkPosition > bottom ? '↓' : '↑'}`) | ||
$(`#${name}-nav-list`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this chained?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to use a different value for the li and a, so I couldn't chain it. I can remove the space, though.
source-browser.html
Outdated
@@ -132,7 +143,7 @@ <h1 id="loading" class="m1 center display-none">⌛</h1> | |||
$('#topics').append( | |||
$('<div class="topic flex-auto">').append( | |||
$('<div>').append( | |||
$('<h2 class="inline-block m0">').text(name), | |||
$('<h2 class="inline-block m0 heading">', {id: name}).text(name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heading
is a super generic class name. How about something like topic-heading
?
source-browser.html
Outdated
#navbar li a { | ||
padding: 14px 16px; | ||
} | ||
.heading { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is off in this entire block.
source-browser.html
Outdated
margin-top: 75px; | ||
margin-bottom: 100px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using 3 margin rules, just use one with 4 values.
source-browser.html
Outdated
@@ -41,8 +52,10 @@ | |||
} | |||
</style> | |||
</head> | |||
<body class="m2"> | |||
<div class="fixed top-0 col-12 py1 mr3 bg-white z1"> | |||
<body class=""> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the empty class attribute.
source-browser.html
Outdated
<body class="m2"> | ||
<div class="fixed top-0 col-12 py1 mr3 bg-white z1"> | ||
<body class=""> | ||
<ul id="navbar" class="fixed m0 p0 overflow-hidden bg-black bottom-0 col-12 z2"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<ul id="navbar" class="fixed m0 p0 overflow-hidden bg-black bottom-0 col-12 z2"> | |
<ul id="navbar" class="fixed m0 p0 overflow-hidden bg-black bottom-0 col-12 z2"></ul> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(i.e. on the same line)
source-browser.html
Outdated
@@ -132,7 +145,8 @@ <h1 id="loading" class="m1 center display-none">⌛</h1> | |||
$('#topics').append( | |||
$('<div class="topic flex-auto">').append( | |||
$('<div>').append( | |||
$('<h2 class="inline-block m0">').text(name), | |||
$('<h2 class="inline-block m0 relative heading">', {id: `${name}-space`}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just id: name
like before? The space
suffix seems odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to use a different id for the empty space above the h2 header, that's why I'm using name-space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you recommend a different name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! Just some tweaks here and there.
source-browser.html
Outdated
@@ -182,6 +193,27 @@ <h1 id="loading" class="m1 center display-none">⌛</h1> | |||
} | |||
|
|||
$(document).ready(() => { | |||
$(window).on('scroll resize load', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's pull the anonymous function here into a function called updateNavbarVisibility
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I didn't mean the whole thing, I meant just the arrow function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just
TOPICS.forEach(({ name }) => { const navLinkPosition = $(
#${name}).position().top; $(
#${name}-nav-link).text(
${name} ${navLinkPosition > bottom ? '↓' : '↑'}); $(
#${name}-nav-list).toggle(top >= navLinkPosition || navLinkPosition >= bottom); });
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned earlier, please use code blocks via 4 space indents.
$(window).on('scroll resize load', updateNavbarVisibility)
is what I'm looking for.
I accidentally resolved the conversation...I have tried the solutions at https://stackoverflow.com/questions/4086107/fixed-page-header-overlaps-in-page-anchors, but they didn't entirely work. The hacky solution was the only solution that has worked entirely, so far. |
These are the CSS solutions. |
for future readers: blocking on merge till I can look into whether there's a cleaner solution to offsetting the anchor jump location |
No description provided.