added fixed navbar by nathanchi · Pull Request #46 · apertium/apertium-on-github

nathanchi · 2018-11-14T06:09:53Z

No description provided.

source-browser.html

sushain97 · 2018-11-14T06:12:37Z

source-browser.html

  </head>
  <body class="m2">
+    <ul class = "navbar" id="navbar">
+      <li><a href="#languages" id="lilang">languages</a></li>


These should not be hardcoded. They should be generated by the JS.

sushain97 · 2018-11-14T06:12:52Z

source-browser.html

          repoUrl: `https://github.com/${ORGANIZATION}/apertium-${topic}`,
        }));
-
+        


Please remove all these extra trailing spaces.

These are not fixed...

I have removed these from throughout the code.

sushain97 · 2018-11-14T06:13:14Z

source-browser.html

            $('<div class="topic flex-auto">').append(
              $('<div>').append(
-                $('<h2 class="inline-block m0">').text(name),
+                $('<h2 class="inline-block m0" id='+name+'>').text(name),


Don't use concatenation. Use a jQuery function to set the id.

sushain97 · 2018-11-14T06:13:52Z

source-browser.html

+        $(window).scroll(function() {
+            var top = $(window).scrollTop();
+            var bottom = $(window).scrollTop() + $(window).height();
+            var langtop = $("#languages").position(); 


These should be adjusted via a loop that goes through the topics, not hardcoded to only work with these.

sushain97

Initial comments. You might find it easiest to run this code through Prettier with the correct settings.

sushain97 · 2018-11-14T17:25:36Z

source-browser.html

    </style>
  </head>
  <body class="m2">
+    <ul class = "navbar" id="navbar"></ul>


Remove spacing around first equal sign.

sushain97 · 2018-11-14T17:25:49Z

source-browser.html

          repoUrl: `https://github.com/${ORGANIZATION}/apertium-${topic}`,
        }));
-
+        


These are not fixed...

sushain97 · 2018-11-14T17:26:30Z

source-browser.html

-
+
+
+        var array = ['languages', 'trunk', 'staging', 'nursery', 'incubator', 'tools'];


This is already defined above as TOPICS.

sushain97 · 2018-11-14T17:26:52Z

source-browser.html

+
+        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 should be no concatenation here.

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.

sushain97 · 2018-11-14T17:27:51Z

source-browser.html

+        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);


What element is this adding to? Why is there an element with this misleading id that sounds like a class?

sushain97 · 2018-11-14T17:30:14Z

source-browser.html

+          $("#inline-block m0").attr("id", name);
+        });
+        $(window).scroll(function() {
+            var top = $(window).scrollTop();


Use only const or let, never var.

sushain97 · 2018-11-14T17:30:45Z

source-browser.html

+            var bottom = $(window).scrollTop() + $(window).height();
+            var titles = ['languages', 'trunk', 'staging', 'nursery', 'incubator', 'tools'];
+            var i;
+            for(i = 0; i<titles.length; i++){


Need spacing between binary operators and i should be declared inline here.

sushain97 · 2018-11-14T17:31:05Z

source-browser.html

+        $(window).scroll(function() {
+            var top = $(window).scrollTop();
+            var bottom = $(window).scrollTop() + $(window).height();
+            var titles = ['languages', 'trunk', 'staging', 'nursery', 'incubator', 'tools'];


Why is this declared again?

sushain97 · 2018-11-14T17:31:46Z

source-browser.html

+            for(i = 0; i<titles.length; i++){
+                var header = titles[i];
+                var vartop = $('#'+header).position();
+                  if(top < vartop.top && vartop.top < bottom){


Please emulate spacing in the rest of this code.

sushain97 · 2018-11-14T17:32:04Z

source-browser.html

+                      if(vartop.top>bottom){
+                          $tools.text(header+'↓')
+                      }
+                      else


Always use braces.

sushain97

Some further suggestions. There are a number of old ones that have not been actioned on either.

sushain97 · 2018-11-15T04:23:02Z

source-browser.html

        EXPIRY_KEY = `${ORGANIZATION}-expiry`,
        EXPIRY_MILLIS = 60000,
-        TOPICS = ['languages', 'trunk', 'staging', 'nursery', 'incubator', 'tools'].map(topic => ({
+        LIST = ['languages', 'trunk', 'staging', 'nursery', 'incubator', 'tools'],


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.

sushain97 · 2018-11-15T04:23:38Z

source-browser.html

        const headers = new Headers({ Accept: 'application/vnd.github.mercy-preview+json' });
        const cacheStale = !(CACHE_KEY in localStorage) || localStorage[EXPIRY_KEY] < Date.now();
-
+          


There are still unnecessary whitespace additions throughout this entire file that should be deleted. Something like 10 of them.

sushain97 · 2018-11-15T04:26:20Z

source-browser.html

            $('<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),


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.

sushain97 · 2018-11-15T04:26:26Z

source-browser.html

            ),
          );
-
+          $("[id=topic]").attr("id", name);


See above comment.

sushain97 · 2018-11-15T04:26:53Z

source-browser.html

+                  else{
+                      $('#li'+heading).show();
+                      let $heading = $('#li'+heading);
+                      if(vartop.top>bottom){


Spacing between binary operations here and throughout should be consistent with existing code.

sushain97 · 2018-11-15T04:28:02Z

source-browser.html

+
      $(document).ready(() => {
+        $("#navbar").append('<li id="li"></li>');
+        for(let i = 0; i < LIST.length; i++){ 


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.

sushain97 · 2018-11-15T04:28:48Z

source-browser.html

+        $("#navbar").append('<li id="li"></li>');
+        for(let i = 0; i < LIST.length; i++){ 
+            $("#li").append('<a id="placeholder"></a>');
+            $("[id=placeholder]").attr({


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.

sushain97 · 2018-11-15T04:31:39Z

source-browser.html

    </script>
  </body>
-</html>
+</html>


Please revert your changes that deleted the final blank line in this file.

sushain97

There are a number of still unresolved comments (at least 3).

sushain97 · 2018-11-16T20:27:07Z

source-browser.html

+          padding: 14px 16px;
+          text-decoration: none;
+      }
+      h2[class$="inline-block m0"] {


sushain97 · 2018-11-16T20:27:38Z

source-browser.html

          name: topic,
          repoUrl: `https://github.com/${ORGANIZATION}/apertium-${topic}`,
        }));
-


Please revert this spacing change and the others.

sushain97 · 2018-11-16T22:30:49Z

source-browser.html

+          padding: 14px 16px;
+          text-decoration: none;
+      }
+      h2[class="inline-block m0"] {


Please look into how to use CSS selectors, especially selecting using classes.

Okay, I'll take a look at that.

I have fixed the CSS.

I'll fix the spacing.

sushain97

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.

sushain97 · 2018-11-16T23:00:52Z

source-browser.html

+      function createNavbar(item) {
+          $('#li').append(
+              $('<a>', {
+                  id: 'li' + item,


Use string interpolation instead of concatenation i.e.

`li${item}`

Also, ids should be id-agnostic if possible. So, something like ${item}-nav-link perhaps?

sushain97 · 2018-11-16T23:00:58Z

source-browser.html

          .focus();
      }

+      function createNavbar(item) {


Suggested change

function createNavbar(item) {

function createNavbar(topic) {

sushain97 · 2018-11-16T23:01:13Z

source-browser.html

+          for(let i = 0; i < TOPICS.length; i++){
+              let heading = TOPICS[i].name;
+              list.push(heading);
+              }


Indentation is off here.

sushain97 · 2018-11-16T23:01:30Z

source-browser.html

      $(document).ready(() => {
+        let list = [];
+        $("#navbar").append('<li id="li"></li>');
+          for(let i = 0; i < TOPICS.length; i++){


Use a forEach instead for consistency.

sushain97 · 2018-11-16T23:01:38Z

source-browser.html

          currentTarget.blur();
          refreshTopics();
        });
-


Revert spacing change here.

sushain97 · 2018-11-16T23:01:57Z

source-browser.html

+              let heading = TOPICS[i].name;
+              let vartop = $('#' + heading).position();
+                if(top < vartop.top && vartop.top < bottom){
+                    $('#li' + heading).hide();


Don't use string ~~interpolation~~ concatenation.

sushain97 · 2018-11-16T23:02:20Z

source-browser.html

+          for(let i = 0; i < TOPICS.length; i++){
+              let heading = TOPICS[i].name;
+              let vartop = $('#' + heading).position();
+                if(top < vartop.top && vartop.top < bottom){


Suggested change

if(top < vartop.top && vartop.top < bottom){

if (top < vartop.top && vartop.top < bottom) {

sushain97 · 2018-11-16T23:02:43Z

source-browser.html

+      $(window).scroll(() => {
+          let top = $(window).scrollTop();
+          let bottom = $(window).scrollTop() + $(window).height();
+          for(let i = 0; i < TOPICS.length; i++){


This should be possible with forEach. Don't create a new function, just use an anonymous arrow function.

sushain97 · 2018-11-16T23:02:53Z

source-browser.html

+                if(top < vartop.top && vartop.top < bottom){
+                    $('#li' + heading).hide();
+            }
+                else{


Spacing and indentation

sushain97 · 2018-11-16T23:03:01Z

source-browser.html

+                else{
+                    $('#li' + heading).show();
+                    let $heading = $('#li' + heading);
+                    if(vartop.top > bottom){


Suggested change

if(vartop.top > bottom){

if (vartop.top > bottom) {

sushain97

Getting close!

sushain97 · 2018-11-17T08:07:47Z

source-browser.html

+          padding: 14px 16px;
+          text-decoration: none;
+      }
+      .inline-block.m0 {


Should not be styling all of them with this. Instead, should use a custom class in the JS and style that.

sushain97 · 2018-11-17T08:08:16Z

source-browser.html

+      ul:nth-child(1) {
+          list-style-type: none;
+          position: absolute;
+          margin: 0;


Many of these rules should be replaced with the basscss v7 rules. e.g. .m0 is margin: 0.

sushain97 · 2018-11-17T08:08:47Z

source-browser.html

    </style>
  </head>
  <body class="m2">
+    <ul class="navbar" id="navbar"></ul>


Just the id should suffice unless you have multiple navbars.

sushain97 · 2018-11-17T08:09:17Z

source-browser.html

          .focus();
      }

+      function createNavbar(topic) {


Inline this function where it is used.

sushain97 · 2018-11-17T08:09:43Z

source-browser.html

+      function createNavbar(topic) {
+        $('.li').append(
+          $('<a>', {
+            id: `li${topic}`,


As I mentioned in my previous review, use a tag-agnostic id.

sushain97 · 2018-11-17T08:11:48Z

source-browser.html

+	    });
+      });
+
+      $(window).scroll(() => {


Should this also happen on window resize?

I've added that, as well.

sushain97 · 2018-11-17T08:12:16Z

source-browser.html

+        let top = $(window).scrollTop();
+        let bottom = $(window).scrollTop() + $(window).height();
+        list.forEach(heading => {
+          let vartop = $(`#${heading}`).position();


What does the var in vartop refer to?

It refers to variable, for each of the heading variables. Would you suggest I rename it?

Does it make sense to name a variable var to indicate that it's a variable? Something like heading_position maybe.

Okay, I'll rename that.

sushain97 · 2018-11-17T08:12:25Z

source-browser.html

+      });
+
+      $(window).scroll(() => {
+        let top = $(window).scrollTop();


Variables that do not change should be const.

Okay, I'll update that tomorrow.

sushain97 · 2018-11-17T08:14:04Z

source-browser.html

+            $(`#li${heading}`).hide();
+          } else {
+            $(`#li${heading}`).show();
+            let $heading = $(`#li${heading}`);


Suggested change

let $heading = $(`#li${heading}`);

$(`#li${heading}`).text(`${heading} ${vartop.top > bottom ? '↓' : '↑'}`);

sushain97 · 2018-11-17T08:14:23Z

source-browser.html

+      });
+
      $(document).ready(() => {
+        $('#navbar').append('<li class="li"></li>');


Why is there an extra list element?

There's only one list element. Would you like me to make this list element hard-coded in the html instead?

Great, I'll do that. :)

nathanchi · 2018-11-17T21:02:39Z

I'm using my dad's computer. That's why the name is different.

sushain97 · 2018-11-17T22:59:26Z

source-browser.html

+      }
+      .topic {
+          padding-top:75px; 
+          margin-top:-75px;


Spacing between colon and value.

sushain97 · 2018-11-17T22:59:44Z

source-browser.html

  </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>


What does this li class do?

This is the hardcoded li class that I added. The goes within the <li>, which goes within the <ul>.

It's used for the .append() method later on in the code, to select only those values.

Why not just #navbar li?.

Unresolved?

sushain97 · 2018-11-17T23:00:30Z

source-browser.html

+            const top = $(window).scrollTop();
+            const bottom = $(window).scrollTop() + $(window).height();
+            TOPICS.forEach(heading => {
+              heading = heading.name;


Use destructuring in the function argument.

sushain97 · 2018-11-17T23:00:38Z

source-browser.html

+            const bottom = $(window).scrollTop() + $(window).height();
+            TOPICS.forEach(heading => {
+              heading = heading.name;
+              let heading_position = $(`#${heading}`).position();


sushain97 · 2018-11-17T23:01:02Z

source-browser.html

+            TOPICS.forEach(heading => {
+              heading = heading.name;
+              let heading_position = $(`#${heading}`).position();
+              if (top < heading_position.top && heading_position.top < bottom) {


If you're only ever using top, just save that instead.

sushain97 · 2018-11-17T23:01:28Z

source-browser.html

      $(document).ready(() => {
+        $(window).on('scroll resize', function(){
+            const top = $(window).scrollTop();
+            const bottom = $(window).scrollTop() + $(window).height();


Use top here instead of another call.

sushain97 · 2018-11-17T23:01:42Z

source-browser.html

+                $(`#${heading}-nav-link`).hide();
+              } else {
+                $(`#${heading}-nav-link`).show();
+                $(`#${heading}-nav-link`).text(`${heading} ${heading_position.top > bottom ? '↓' : '↑'}`);


These calls can be chained.

sushain97 · 2018-11-17T23:01:58Z

source-browser.html

+          });
+
+        TOPICS.forEach(topic => {
+            topic = topic.name;


Destructuring here as well.

sushain97 · 2018-11-17T23:02:38Z

source-browser.html

+            topic = topic.name;
+            $('.li').append(
+              $('<a>', {
+                class: `inline-block white center btn:hover`,


I would just add this to the string part since there's no interpolation.

sushain97 · 2018-11-17T23:03:03Z

source-browser.html

+        $(window).on('scroll resize', function(){
+            const top = $(window).scrollTop();
+            const bottom = $(window).scrollTop() + $(window).height();
+            TOPICS.forEach(heading => {


I think append can take a list, right? In that case, use a map to create the list and append that.

Which append are you referring to?
$('.li').append( ?

From what I understand, that area of the code doesn't need appending.

sushain97 · 2018-11-18T07:36:11Z

source-browser.html

+        $(window).on('scroll resize', function(){
+            const top = $(window).scrollTop();
+            const bottom = top + $(window).height();
+            TOPICS.forEach(topic => {


Suggested change

TOPICS.forEach(topic => {

TOPICS.forEach(({name}) => {

sushain97 · 2018-11-18T07:36:54Z

source-browser.html

+            const bottom = top + $(window).height();
+            TOPICS.forEach(topic => {
+              let {name} = topic;
+              const name_position = $(`#${name}`).position().top;


Suggested change

const name_position = $(`#${name}`).position().top;

const navLinkPosition = $(`#${name}`).position().top;

sushain97 · 2018-11-18T07:37:08Z

source-browser.html


      $(document).ready(() => {
+        $(window).on('scroll resize', function(){
+            const top = $(window).scrollTop();


The rest of the code is indented two spaces but now it's 4?

I'll run it through Prettier again, then.

sushain97 · 2018-11-18T07:37:15Z

source-browser.html

      }

      $(document).ready(() => {
+        $(window).on('scroll resize', function(){


Suggested change

$(window).on('scroll resize', function(){

$(window).on('scroll resize', () => {

sushain97 · 2018-11-18T07:38:56Z

source-browser.html

+              if (top < name_position && name_position < bottom) {
+                $(`#${name}-nav-link`).hide();
+              } else {
+                $(`#${name}-nav-link`).text(`${name} ${name_position > bottom ? '↓' : '↑'}`).show();


Can replace the entire block with something like:

Suggested change

$(`#${name}-nav-link`).text(`${name} ${name_position > bottom ? '↓' : '↑'}`).show();

$(`#${name}-nav-link`)

.text(`${name} ${name_position > bottom ? '↓' : '↑'}`)

.toggle(top >= name_position || name_position >= bottom);

sushain97 · 2018-11-18T07:39:39Z

source-browser.html

+              }
+            });
+          });
+        TOPICS.forEach(topic => {


This entire block can be replaced with $('.li').append(TOPICS.map(({name}) => ...

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>.

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?

Nevermind, it works now.

sushain97

Almost there!

sushain97 · 2018-11-19T07:23:21Z

source-browser.html

+
+        TOPICS.map(({name}) => {
+          $('#navbar').append(
+            $('<li class = "left">').append(


Suggested change

$('<li class = "left">').append(

$('<li class="left">').append(

sushain97 · 2018-11-19T07:24:41Z

source-browser.html

+        });
+
+        TOPICS.map(({name}) => {
+          $('#navbar').append(


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.

So something like this?
$('#navbar').append(
TOPICS.map(({ name }) => {

The js syntax seems to not work when I do that, however. Do you have any suggestions on how to do this?

It should work. What is the error and where is it occurring?

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.

Do you know what the error might be?

You shouldn't have multiple semicolons in one statement. That is, your first semicolon should probably be removed.

I edited the jquery as you suggested, and now the code looks like this:

https://pastebin.com/T1RPg36n

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.

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.

Alright, I've fixed it.

sushain97 · 2018-11-19T07:25:04Z

source-browser.html

+        TOPICS.map(({name}) => {
+          $('#navbar').append(
+            $('<li class = "left">').append(
+              $('<a class = "inline-block white center btn:hover">', {


Suggested change

$('<a class = "inline-block white center btn:hover">', {

$('<a class="inline-block white center btn:hover">', {

sushain97 · 2018-11-19T07:25:34Z

source-browser.html

+              $('<a class = "inline-block white center btn:hover">', {
+                id: `${name}-nav-link`,
+                href: `#${name}`,
+                class: `inline-block white center btn:hover`


This isn't needed anymore?

I'm sorry, I'm not quite sure if I understand. Why don't I need it?

You already have it above in the string.

On 200? It doesn't define the id, href, or class.

I'm not saying the id and href aren't needed. This comment is on the line with the class.

Oh, I'm sorry, I misunderstood what you meant.

sushain97 · 2018-11-19T07:26:02Z

source-browser.html

+
        $('#github-link').attr('href', `https://github.com/${ORGANIZATION}`);
-        $('#refresh').click(({ currentTarget }) => {
+        $('#refresh').click(({currentTarget}) => {


Can you use the proper prettifier settings to revert this change and also use the same spacing elsewhere?

sushain97 · 2018-11-19T07:26:38Z

source-browser.html

    </style>
  </head>
  <body class="m2">
+    <ul class="fixed m0 p0 overflow-hidden bg-black bottom-0 col-12 z2" id="navbar">


Suggested change

<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>

sushain97

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?

sushain97 · 2018-11-20T17:15:30Z

source-browser.html

+			}).text(name)
+			),
+		  ),
+		);


Spacing is very suspicious here. Try running this through Prettier.

nathanchi · 2018-11-20T23:24:11Z

The navbar is 2 pixels offset from the left, because
<ul id="navbar" class="fixed m0 p0 overflow-hidden bg-black bottom-0 col-12 z2">
is under
<body class="m2">
which gives it a margin of 2. Should set the margin to m0 instead? This would resolve the issue, but the margin would be gone.

nathanchi · 2018-11-20T23:32:54Z

Nevermind, I got it to work! :)

nathanchi · 2018-11-21T01:42:37Z

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.

nathanchi · 2018-11-21T01:44:22Z

Currently, I'm using

.heading {
padding-top: 75px;
margin-top: -75px;
}

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...

nathanchi · 2018-11-21T01:45:46Z

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.

sushain97

Some extra comments and there's a comment from before re. chaining that hasn't been resolved.

sushain97 · 2018-11-21T03:51:17Z

source-browser.html

+          margin-top: -75px;
+      }
      #content {
+		margin: 1rem;


sushain97 · 2018-11-21T03:51:42Z

source-browser.html

+            const navLinkPosition = $(`#${name}`).position().top;
+            $(`#${name}-nav-link`)
+              .text(`${name} ${navLinkPosition > bottom ? '↓' : '↑'}`)
+            $(`#${name}-nav-list`)


Why isn't this chained?

I had to use a different value for the li and a, so I couldn't chain it. I can remove the space, though.

sushain97 · 2018-11-21T03:53:07Z

source-browser.html

            $('<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),


heading is a super generic class name. How about something like topic-heading?

sushain97 · 2018-11-22T07:45:42Z

source-browser.html

+      #navbar li a {
+        padding: 14px 16px;
+      }
+	  .heading {


Indentation is off in this entire block.

sushain97 · 2018-11-22T07:46:00Z

source-browser.html

      #content {
+        margin: 1rem;
        margin-top: 75px;
+        margin-bottom: 100px;


Instead of using 3 margin rules, just use one with 4 values.

sushain97 · 2018-11-22T07:46:29Z

source-browser.html

  </head>
-  <body class="m2">
-    <div class="fixed top-0 col-12 py1 mr3 bg-white z1">
+  <body class="">


Please remove the empty class attribute.

sushain97 · 2018-11-22T07:46:36Z

source-browser.html

-  <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">


Suggested change

<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>

(i.e. on the same line)

sushain97 · 2018-11-22T07:47:17Z

source-browser.html

            $('<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`}),


Why not just id: name like before? The space suffix seems odd.

I need to use a different id for the empty space above the h2 header, that's why I'm using name-space.

Would you recommend a different name?

sushain97

Almost there! Just some tweaks here and there.

source-browser.html

sushain97 · 2018-11-22T18:29:54Z

source-browser.html

      }

      $(document).ready(() => {
+        $(window).on('scroll resize load', () => {


Let's pull the anonymous function here into a function called updateNavbarVisibility.

This has been fixed.

No, I didn't mean the whole thing, I meant just the arrow function.

Okay, I'll fix that.

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); });?

As I mentioned earlier, please use code blocks via 4 space indents.

$(window).on('scroll resize load', updateNavbarVisibility)

is what I'm looking for.

source-browser.html

nathanchi · 2018-11-23T07:10:43Z

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.

nathanchi · 2018-11-23T07:10:57Z

These are the CSS solutions.

source-browser.html

sushain97 · 2018-11-24T02:43:17Z

for future readers: blocking on merge till I can look into whether there's a cleaner solution to offsetting the anchor jump location

Nathan Chi added 2 commits November 13, 2018 21:00

added fixed, edited navbar to source browser

748a10f

fixed spacing

1a44b6b

sushain97 requested changes Nov 14, 2018

View reviewed changes

Nathan Chi added 2 commits November 13, 2018 22:16

fixed extra spacing

e9595a6

fixed up navbar

faf010d

sushain97 requested changes Nov 14, 2018

View reviewed changes

fixed spacing, edited js/jquery and attributes

dd265b8

sushain97 requested changes Nov 15, 2018

View reviewed changes

sushain97 reviewed Nov 15, 2018

View reviewed changes

edited spacing and jquery/js

e778e69

sushain97 requested changes Nov 16, 2018

View reviewed changes

Nathan Chi added 2 commits November 16, 2018 14:15

edited spacing, css

4103dfb

fixed spacing, reverted spacing changes

ab7d0a9

sushain97 requested changes Nov 16, 2018

View reviewed changes

Nathan Chi added 2 commits November 16, 2018 14:36

edited css

c6fb817

removed extra trailing spaces

e9e36e5

sushain97 requested changes Nov 16, 2018

View reviewed changes

Nathan Chi added 2 commits November 16, 2018 18:01

edited spacing, ran through Prettier, edited JS/JQuery

80d0351

edited spacing, script, ran through Prettier

4a8f35f

sushain97 requested changes Nov 17, 2018

View reviewed changes

edited script

a9e5cfe

sushain97 requested changes Nov 17, 2018

View reviewed changes

fixed script

6377cb9

sushain97 requested changes Nov 18, 2018

View reviewed changes

Square Snow added 2 commits November 18, 2018 21:28

ran through prettier to fix styling, edited script

6aed63a

edited spacing

71b2028

sushain97 requested changes Nov 19, 2018

View reviewed changes

Nathan Chi added 2 commits November 19, 2018 17:58

edited script and spacing

a2c8ce9

edited jquery/js script

53fbcf4

sushain97 requested changes Nov 20, 2018

View reviewed changes

source-browser.html Outdated

}).text(name)

),

),

);

Copy link

Member

sushain97 Nov 20, 2018

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.

ran through prettier

d1527a3

edited appearance

920281d

Nathan Chi added 2 commits November 21, 2018 16:56

edited anchor links

897728e

edited navbar

8c7f959

sushain97 requested changes Nov 22, 2018

View reviewed changes

Nathan Chi added 2 commits November 22, 2018 00:27

edited details

98f71a1

edited spacing

c5a4337

sushain97 requested changes Nov 22, 2018

View reviewed changes

Nathan Chi and others added 4 commits November 23, 2018 00:26

fixed issues

4f1502a

edited spacing

7bafb7f

fixed more spacing issues

773d925

spacing

3e5f3d3

sushain97 reviewed Nov 24, 2018

View reviewed changes

source-browser.html Show resolved Hide resolved

Nathan Chi added 2 commits November 23, 2018 18:01

edited spacing

d1d50a9

edited jquery/js

764b4b6

sushain97 requested changes Nov 24, 2018

View reviewed changes

source-browser.html Outdated Show resolved Hide resolved

source-browser.html Show resolved Hide resolved

source-browser.html Show resolved Hide resolved

edited script

05c0894

		repoUrl: `https://github.com/${ORGANIZATION}/apertium-${topic}`,
		}));




		var array = ['languages', 'trunk', 'staging', 'nursery', 'incubator', 'tools'];

		const headers = new Headers({ Accept: 'application/vnd.github.mercy-preview+json' });
		const cacheStale = !(CACHE_KEY in localStorage) \|\| localStorage[EXPIRY_KEY] < Date.now();

	if(top < vartop.top && vartop.top < bottom){
	if (top < vartop.top && vartop.top < bottom) {

	let $heading = $(`#li${heading}`);
	$(`#li${heading}`).text(`${heading} ${vartop.top > bottom ? '↓' : '↑'}`);

	const name_position = $(`#${name}`).position().top;
	const navLinkPosition = $(`#${name}`).position().top;

	$(window).on('scroll resize', function(){
	$(window).on('scroll resize', () => {

	$('<li class = "left">').append(
	$('<li class="left">').append(

	$('<a class = "inline-block white center btn:hover">', {
	$('<a class="inline-block white center btn:hover">', {

	<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>

Conversation

nathanchi commented Nov 14, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sushain97 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sushain97 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sushain97 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sushain97 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sushain97 Nov 16, 2018 •

edited

Loading