Resolved Issue #33 #34
Conversation
index.html
Outdated
| @@ -34,10 +34,10 @@ | |||
| <div class="container"> | |||
| <h1 class="display-4">Commons Mobile App</h1> | |||
| <a class="app-badge f-droid-badge" href="https://f-droid.org/repository/browse/?fdid=fr.free.nrw.commons"> | |||
| <img src="images/f-droid-badge.png" alt="F-Droid Logo" /> | |||
| <img src="https://raw.githubusercontent.com/commons-app/commons-app.github.io/master/images/f-droid-badge.png" alt="F-Droid Logo" /> | |||
There was a problem hiding this comment.
What's the advantage of using an absolute link to GitHub instead of a relative link which works just fine?
There was a problem hiding this comment.
Both are just fine for me. I used codepen.io to add the smooth scrolling effect, so I placed an absolute link there. If you want I can change it back to relative.
There was a problem hiding this comment.
Please revert them to relative paths itself.
index.html
Outdated
| </div> | ||
| </footer> | ||
|
|
||
| <script src="https://code.jquery.com/jquery-3.3.1.slim.min.js" integrity="sha384-q8i/X+965DzO0rT7abK41JStQIAqVgRVzpbzo5smXKp4YfRvH+8abtTE1Pi6jizo" crossorigin="anonymous"></script> | ||
| <script src='https://cdnjs.cloudflare.com/ajax/libs/jquery/3.1.1/jquery.min.js'></script> |
There was a problem hiding this comment.
I'm asking this as I'm not aware. Is there an advantage of using the Cloudflare CDN rather than JQuery URL?
Further, you seem to have switched to a older version of jQuery when changing the URL.
| }); | ||
|
|
||
| // Active link switching | ||
| $(window).scroll(function() { |
There was a problem hiding this comment.
I generally think it would be nice to achieve something simple as this without custom JS (possibly using CSS; there's already a PR (#32) which does just this).
Not using JS has the advantage that it doesn't require maintainence :-)
There was a problem hiding this comment.
I also thought of using CSS initially but for time delay and stuff, js is required.
If I found something that can be done via css then I'll add a PR.
| </head> | ||
|
|
||
| <body> | ||
| <nav class="navbar navbar-expand-lg navbar-dark bg-primary"> | ||
| <nav class="navbar fixed-top navbar-expand-lg navbar-dark bg-primary"> |
There was a problem hiding this comment.
I could see the reason why you're using fixed-top for the navbar but it seems to be making the text ("Commons Mobile App") within the <h1> contained within <div class="jumbotron jumbotron-fluid"> to be very close to the navbar in the desktop view and it even goes under the navbar in mobile view. Can you fix this?
There was a problem hiding this comment.
BTW, sorry for the late request of change. Couldn't get my hands on this for a while :-(
There was a problem hiding this comment.
I managed to fix this issue in 4ff0f4c
We just have one file. A folder isn't worth it.
As the navbar has been made fixed we have to compensate it with a little top padding for the body as the size of the navbar isn't taken into account when it is turned into fixed. Ref: https://stackoverflow.com/a/11124805/5614968
|
Thanks a lot for your contribution @soulspark666! Hope to see you again :-) |
Added smooth scrolling view according to issue #33