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

Modularize frontend with Jekyll #28

Merged
merged 1 commit into from Feb 17, 2018

Conversation

Projects
None yet
2 participants
@bgiesing
Contributor

bgiesing commented Feb 13, 2018

After I updated the UI back in July 2017 with the newer Bootstrap, I've been thinking about how else to improve the site and one of the things was "modularizing" the site by reusing chunks with Jekyll (Same thing powering GitHub Pages).

The advantages of this include:

  1. Cross-site things like the navbar, footer, and head tags are all in one place. This allows easier editing and should prevent accidentally forgetting to update a specific page.
  2. Easier page creation, just use an existing template.
  3. Less duplication so code should be smaller by not having the same stuff on every page (All the pages had the exact same head tags like Bootstrap other than a few differences which could be handled with if statements) or by making variables (The Bootstrap CDN urls are all similar, could replace it with a variable to cut off a chunk of characters and make it easy to update the version number for all at once)
  4. Use Markdown to easily format things usually done by more complex HTML tags (like the Image links on the homepage can be replace from long nested tags to simplier nested links)

Disadvantages:

  1. Have to build the site before putting it into the server JAR. You should be able to figure out how to automate this.
  2. Having to install more stuff to build the site.

This is far from complete at the moment but it should still function identically to the current site assuming I didn't screw up badly. I can't really test on my end though as nothing will play back, which I think might be my end setup wrong so I'm posting this even if unfinished for you to test and see if anything needs to be fixed I haven't noticed, see if anything needs fixed on the Kotlin/JAR side (I don't know anything about that so IDK what would need fixed if anything), and of course for you to decide if this isn't needed and the problem shouldn't be tackled at all or done in a different way.

Testing instructions:

  1. Install Jekyll
  2. Generate the site (use jekyll serve -d web -s _web/)
  3. Set the webRoot in the config.json file to web
  4. Place JAR and config into the web directory.
  5. Run with the JAR like normal.

This change is Reviewable

@bgiesing

This comment has been minimized.

Contributor

bgiesing commented Feb 13, 2018

Updated a whole bunch of stuff in the past 3 hours:

  1. Used Data files for Navbar, FAQ, etc. This allows them to be made automatically with way less code just by adding some simple Markdown/YAML.
  2. Converted the Profile and finished the FAQ page like I said I needed to
  3. Moved the entire Retro site into Jekyll also with some slight tweaks (Header/Footer on FAQ page, FAQ uses same Data file as the modern site so both update automatically, "Return to Modern Jukebox" link)

Still definitely needs tested as this is a complete structure overhaul but I'm pretty sure 99% of everything is converted over and working.

@bgiesing

This comment has been minimized.

Contributor

bgiesing commented Feb 14, 2018

More updates:

  1. Auto-compress the final generated site with a layout that that trims whitespace, removes optional end/start tags, and more.
  2. Made a Search and Go layout since both the Canonizer and Jukebox were nearly identical other than a few lines (which were handled with if statements)
  3. Various fixes and cleanup

@bgiesing bgiesing force-pushed the bgiesing:jekyll-rewrite branch 2 times, most recently from 13830b4 to a9bb65d Feb 14, 2018

@bgiesing

This comment has been minimized.

Contributor

bgiesing commented Feb 14, 2018

Rebased everything to remove some fixup commits and include the changes of 5475ca9 on the rewrite branch

@UnderMybrella

This comment has been minimized.

Owner

UnderMybrella commented Feb 15, 2018

Hey, this looks great! Couple of quick things:

  1. Once you bring it in line with the latest changes, I'll test it locally, but from what I've done so far it seems to be working pretty well. (There were a few HTML changes, particularly in regards to API call and audio uploading)
  2. The dependency isn't an issue, no problems there.
  3. While you're doing HTML changes, if possible, would you be able to look at the tuning window's positioning? For whatever reason, it gets stuck under the main content, while it should (ideally) be floating. Don't know how easy/if it's possible to fix, but if you could that would be great.
  4. If possible, moving the Jekyll files from web to a folder like jekyll (or even the _web denoted now as the destination) would be great, since the folder isn't usable in of itself, and it copies all the relevant files over as well. If there's an issue here that's fine, let me know and I'll work with it.

So far this looks really nice though, thanks.

@bgiesing

This comment has been minimized.

Contributor

bgiesing commented Feb 15, 2018

Thanks for the feedback!

  1. Don't have time at the moment (getting ready to live stream in about an hour) but I'll get to that asap.
  2. 👍
  3. I'll try to look at it, not sure if I can fix it.
  4. That totally would be possible. Basically just gotta rename web to _web and you would have to run a modified build command (to force Jekyll to not build into the normal _site directory) like jekyll serve -d web -s _web/ (basically swapping the two outputs from the command I put in the original post so files get outputted to web instead of _web)

@bgiesing bgiesing force-pushed the bgiesing:jekyll-rewrite branch 3 times, most recently from 0ac7eca to fe34b6b Feb 16, 2018

@bgiesing

This comment has been minimized.

Contributor

bgiesing commented Feb 16, 2018

I don't have time to look into the tuning window right now but I went ahead and rebased it on a17197a of the rewrite branch and changed web to _web per your suggestion so web remains the final copy. I updated .gitignore to reflect that.

I also squashed everything into one commit so it should be easier to rebase again if needed.

@UnderMybrella

This comment has been minimized.

Owner

UnderMybrella commented Feb 16, 2018

Thanks for all your work, I'll check this out in a moment. Apart from the tuning window, is everything else done or should I wait longer after testing?

@bgiesing

This comment has been minimized.

Contributor

bgiesing commented Feb 16, 2018

Everything else should be done unless you find something I screwed up on XD

@UnderMybrella

This comment has been minimized.

Owner

UnderMybrella commented Feb 16, 2018

Overall it's pretty good, few issues:

  1. The profile page flat out does not work. The profile request is malformed, and so does not execute.
  2. The canonizer_go page does not work; you can't play audio. The page loads and formats, but never plays. There are two reasons for this - the header points to jremix.js rather than canonizer_jremix.js which is slightly different. As a result, the player doesn't execute properly. On top of that, the ID for the play button handler is different between the canonizer and jukebox (play vs go) and both have the ID of the button as go.
  3. The search page leads to the canonizer regardless of whether it's canonizer or jukebox
  4. The formatting for the search page is a little weird; the header and results are in different indents.

Apart from that, everything seems to be working for the most part, thanks once more

@bgiesing

This comment has been minimized.

Contributor

bgiesing commented Feb 16, 2018

  1. Not sure how to fix that, the profile is the one page I basically didn't touch at all.
  2. That should be an easy fix, just need to implement an if statement like I did for a lot of other things.
  3. Should be similar to 2
  4. Not sure why it is, maybe some issue issue with pages now sharing the same head contents, some CSS might be conflicting.

I don't have time to fix these tonight but I'll look into them tomorrow.

@UnderMybrella

This comment has been minimized.

Owner

UnderMybrella commented Feb 16, 2018

That's alright, take your time. The profile should just be recopying the javascript, but if it doesn't work I'll fix it on my end.

@bgiesing bgiesing force-pushed the bgiesing:jekyll-rewrite branch 3 times, most recently from 1489e7e to d8dcc15 Feb 17, 2018

Rewrite the site to use Jekyll
This allows modular component reuse on the frontend (like the
header/footer) without putting it into every single page.

It has to be generated first with Jekyll (make sure you have Jekyll
export it into the server directory the jar uses).

Use For-loop for FAQ

Move Variables to data file

- Also move _config.yml into web folder
- Fix CDN var
- Add var for the GitHub

Add Liquify Plugin

Fixes the Git links since they Liquid variables can't be used in Yaml
files like the data set used for the FAQ

Navbar by Looping

Simplify adding Nav Bar items by doing a for loop with a Data file

Split Tweet button into include

Convert Profile page into Jekyll

Convert Retro into Jekyll

- FAQ now loads the same content as the updated site but in the style of
the old page
- Added Header/Footer to FAQ to make it easier to navigate.
- Added a "Return to Modern Jukebox" link to Retro Footer since Retro
doesn't have the dropdown nav.

Add Compression

Add a layout that auto-compresses the page code on generation by
trimming whitespace, removing optional end/start tags, and more.

Remove Duplicate Jquery line in Retro
Also a couple blank lines

Move Rufous iframe to Default layout

Go and Search Layout

Go and Search pages are very similar between the Jukebox and Canonizer
other than a few minor things (like API endpoint on the Search pages)

Also updated the button panel to be made dynamically with YAML in the
layout (no separate data file since there's so little items)

@bgiesing bgiesing force-pushed the bgiesing:jekyll-rewrite branch from d8dcc15 to f7ac9fc Feb 17, 2018

@bgiesing

This comment has been minimized.

Contributor

bgiesing commented Feb 17, 2018

1. Not sure if the Profile issue is fixed, I recopied everything and reverted profile to stop using the head include just in case some Head stuff was conflicting. Can't test on my end cause the JAR server just says Resource not found even though the page is there.

2 and 3. Should be fixed

4. This was the biggest issue and I finally figured out how to fix it after quite a while. The layout screwup was because both styles.css and canonizer_styles.css were loading on every page. I had to tweak the code to only put styles.css on the Jukebox Go page (since that's the only page that uses it other than the retro pages (which the head is already handled entirely separate since the Retro code is so different))

Speaking of the Retro Jukebox, that's a good segway into some other changes I made:

  1. I added the Nav to the Retro site. This way it's way easier to switch between the 3 "apps" and maintains better consistency across the site. Everything else remains completely untouched so the only difference is everything sifted a few pixels for the nav which I think is a nice compromise between "leaving it completely untouched" and "improving usability".
  2. I switched all the IF statements that detected if it was the Canonizer or Jukebox from using Page title to a separate "app" variable. Not only is this shorter (code smaller and easier to read) but it allows for possibilities in the future to change the page titles without completely breaking the site. (Like the Go pages could make the title include the song name)
  3. I fixed the tuning window's position. Had to add some CSS to both styles.css files and make sure the files came after jQuery UI to overwrite it's defaults.
  4. Turned the jQuery CDN URL into a variable also (like Bootstrap) and switched jQuery UI to use jQuery CDN instead of Google's so they both use the same variable.

Assuming everything works now, I would consider this ready to merge.

@bgiesing bgiesing changed the title from Modularize frontend with Jekyll [WIP DO NOT MERGE] to Modularize frontend with Jekyll Feb 17, 2018

@UnderMybrella UnderMybrella merged commit c2a587a into UnderMybrella:rewrite Feb 17, 2018

1 check passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
@UnderMybrella

This comment has been minimized.

Owner

UnderMybrella commented Feb 17, 2018

Nicely done; there's a few small tweaks I'll have to make due to a few backend changes, but thanks so much for all your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment