Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redesign homepage by upgrading to Bootstrap 5 #810

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

veyndan
Copy link
Contributor

@veyndan veyndan commented Jul 31, 2023

json-ld.org is using Bootstrap 2.x, which is about 10 years old as of now. In an attempt to give the website a fresh lick of paint, I upgraded the homepage website from Bootstrap 2.x to 5.3.1.

As I only ugpraded the Bootstrap version for the homepage in this PR, navigating between sub pages will look a bit odd, as the other pages are still using Bootstrap 2.x. I wanted to create this PR so I can get opinions on whether such a change would be accepted, before I go and upgrade the whole site to 5.x!

PS: One notable difference is that the icons have been removed. Glyphicons are no longer bundled within Bootstrap, and from what I can tell, getting them from Glyphicons directly costs money. If we wanted to re-add the icons, we would need to get it from another source (e.g., Font Awesome or Bootstrap Icons). I tried using Bootstrap Icons, but I found the icons to look out of place in the redesign, so decided to just rip them out.

Before After
image image
image image
image image
image image

@gkellogg
Copy link
Member

Personally, I think we've accrued too much technical debt on this site, so I appreciate your efforts to move things forward. I think the site works well without the icons, but if we decide on this direction, it would be worth looking at either Font Awesome, or to see about using native Bootstrap icons.

@davidlehn
Copy link
Member

Thank you for an update. The site is indeed ancient, but has held up well!

  • The entire site needs an overhaul, beyond CSS. It needs to use a modern static site builder. There was an attempt back in Jekyll-based json-ld.org #461. That should be revisited. There was some chatter about using eleventy instead.
  • A major static site update would move the CSS updates to a single global theme. I recommend waiting to do that rather than updating all the pages by hand. Other than the playground, the site is not complex and I assume modern content design and theme systems of most site generators would work fine.
  • If/when the site is updated, it needs to take into account building the playground as a SPA with modern tooling. I was doing some eleventy experiment with that but didn't finish.
  • The updated bootstrap looks fine, but I do like icons. I'd use fontawesome. And I think the playground icon is critical. :-)

@veyndan
Copy link
Contributor Author

veyndan commented Aug 2, 2023

@davidlehn Thanks for the extra context!

  • A major static site update would move the CSS updates to a single global theme. I recommend waiting to do that rather than updating all the pages by hand. Other than the playground, the site is not complex and I assume modern content design and theme systems of most site generators would work fine.

I'm a bit confused by this point, as there are no CSS updates in this PR, except for obviously referencing CSS that is within Bootstrap.

  • The updated bootstrap looks fine, but I do like icons. I'd use fontawesome. And I think the playground icon is critical. :-)

Happy to put the icons back in, though I still think it looks a bit odd. Will update the PR with them and let you decide.

All in all, what are the immediate next steps with this? Does it make sense for me to continue updating the rest of the website to Bootstrap 5? The homepage at least wasn't too difficult to update, so I'm happy to have a crack at updating the other pages, even if it is just an interim solution until/if we upgrade to some theme system from a site generator.

@davidlehn
Copy link
Member

By CSS I meant the general theme of the site. Main layout HTML, CSS, menus, etc, should be in one theme in one place. There should be no need to update pages by hand. This site predates mostly all modern static site generators and we've never tackled the update. I'm suggesting not spending the time with manual updates since that will all get dropped when a better system is adopted. The updates done here could be a base for that updated theme. It should only need to be done once and every page will inherit it.

@veyndan
Copy link
Contributor Author

veyndan commented Aug 3, 2023

@davidlehn Thanks for clearing things up. Judging by #461, that effort has taken/is taking a long time, and I don't particularly want to embark on rehauling the whole site architecture (at least at the moment). Upgrading to Boostrap 5 isn't much effort, as its mostly a find-and-replace job of updating class names.

My original motivations for this PR are to:

  1. Reduce how much we're sending over the wire. Many files depend on both a CDN version of Bootstrap AND a local version of Bootstrap. We should only need one of them. Instead of fighting some old version of Bootstrap to remove the local version, it's easier to just upgrade Bootstrap in its entirety. Bootstrap 5 also removes its dependency on JQuery.
  2. Make it easier to read the website. Currently, I find it fairly difficult to read the website, and with the updated Bootstrap (and some styling changes), I'm finding it a lot easier.

What you're mentioning seems to also make it easier to contribute to the website. I definitely think this is important, but can be done in a separate PR via the usage of some static site generator.

If it's just a concern of increased effort from my part, then I'll happily continue working on this, because I'd rather sink some time into upgrading to Bootstrap 5 and having it merged, than migrating over to 11ty or Jekyll and have the PR sit open for a long time.

Copy link
Member

@gkellogg gkellogg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @veyndan! Even though the website has needed a complete redesign, that effort has long been stalled. In the mean time, making incremental improvements, such as updating the bootstrap version, goes a long way towards reducing the technical debt.

@veyndan
Copy link
Contributor Author

veyndan commented Aug 5, 2023

@gkellogg @davidlehn I've upgraded the other pages to use Bootstrap 5. Please see the updated screenshot table!

  1. I've re-included the icons for the section headers on the homepage. Please lmk if you prefer the design with them or without (I'm still on the side of without).
  2. The playground was a bit fiddly to upgrade, so please check it out and make sure it's working as expected. I already did a bit of manual testing on it, but both of you probably have a lot more experience with the tool than I do 😄
  3. Let me know if there are any other sub pages that should be upgraded and that I missed. The pages with a Bootstrap dependency in which I didn't upgrade are:
    a. playground/dev/index.html
    b. primer/index.php
    c. requirements/index.php
    d. spec/index.php
    e. test-suite/index.html
    f. test-suite/vocab.html
    g. test-suite/vocab_template.haml

@gkellogg
Copy link
Member

gkellogg commented Aug 5, 2023

I can live without the icons, and it actually gives the site a bit more modern look to not have them.

The playground is also a bit fiddly, and really needs its own overhaul (see various issues).
I don't think those other pages need to be updated.

  • The test suite has been moved, and we should probably just remove these files from this repo.
  • The dev playground is also obsolete, as it was there prior to the switch to JSON-LD 1.1
  • The requirements and primer are also also obsolete.
  • dev/index.php can be accessed directly, but I don't think it is directly accessable through site navitagation. That said, it serves a purpose and should eventually be updated.

@veyndan
Copy link
Contributor Author

veyndan commented Aug 5, 2023

I can live without the icons, and it actually gives the site a bit more modern look to not have them.

Awesome! I just removed them in the latest commit.

Thanks for the detailed list of follow ups. Deleting code is always fun, so I'll happily pick that up after this PR is merged.

dev/index.php can be accessed directly, but I don't think it is directly accessable through site navitagation. That said, it serves a purpose and should eventually be updated.

I went to https://json-ld.org/dev but am getting a 404. Are you referring to a different dev/index.php?

@gkellogg
Copy link
Member

gkellogg commented Aug 5, 2023

That should be spec/index.php. https://json-ld.org/spec/

veyndan added a commit to veyndan/json-ld.org that referenced this pull request Aug 6, 2023
@veyndan veyndan mentioned this pull request Aug 6, 2023
@veyndan
Copy link
Contributor Author

veyndan commented Aug 6, 2023

@gkellogg Just updated spec/index.php to Bootstrap 5 in the latest commit.

@BigBlueHat
Copy link
Member

As the author of #461 (which got bogged down in fiddly complexity...), I'm in favor of getting this merged to move forward (as I don't want this PR to end up in the same state that my hard work did).

We can revisit the move to 11ty in #461 (and deal with the list of "blockers" in this comment) over there or in separate issues/PRs.

Mainly, we just need this site to move forward for the good of the community and the Web, so thank you @veyndan this PR! Let's merge it!

@gkellogg gkellogg requested a review from davidlehn August 7, 2023 16:11
@BigBlueHat
Copy link
Member

@davidlehn can we discuss this one soon? I don't want it to end up in the same fate as #461.

@gkellogg
Copy link
Member

I think we just need to merge and move on. It makes a definite improvement. @davidlehn?

@davidlehn
Copy link
Member

I'm ok with the bootstrap update in theory. However I'm not as enthusiastic about the arbitrary style changes. And there are quite a few problems. Did others test this out or only look at the images? I fired this up locally to compare side by side.

@veyndan: Can you please fix the conflicts. Some updates I did caused them, my bad.

  • The diffs are large and hard to compare. I have no idea how to review if they are doing the correct things.
  • font: The main font I see is wrong. It's using something where the JSON cap "J" drops below the baseline. And while I'm typing this in github, it does it here too. So I guess I have a weird browser default? The current site has something else set and I think is more correct for the title and such. (I'm not a font person, sorry for not knowing font style names here.) The screenshots do not seem to have this issue.
  • links: Do people like the plethora of bright blue underlined links? That seems to be a bootstrap 5 default. It's very blue. Perhaps too much blue? I'm not sure the old red non-underlined was better or not? It does remind me of classic pre-CSS default HTML beauty.
  • playground: context warning UI is displayed on load by mistake. It goes away as soon the main processing action is called, ie on a keystroke. Should be hidden and displayed on demand as before.
  • general: Seems most br break lines are gone? Should they be? I had thought those were nice. These days perhaps people do that with sections and css.
  • playground: The error section removed the red alert background, it's just plain text. I think the alert color and box is important.
  • playground: Input/options section is inset on each side. It should be flush to the same edge as the other sections as before.
  • playground: The nav, title, text, and notes take up more vertical space than before. Kind of an issue since the input/output sections are the most important and better if more of that displays by default. May be difficult to micro-adjust just the spacing/fontsize/etc for that though.
  • playground: All the bright blue buttons and tabs look terrible. It looks like bootstrap does this on purpose from their nav/tab docs? A strange choice to make everything look bad by default. Is there a better way? I did like the grayed out inactive tabs.
  • playground: The "Table" output is no longer in a scrollable container and for output where it expands too wide, the whole page has to be scrolled.
  • playground: The Options radio options are all bunched together with no margin, and they are all offset up higher than they should be from the left label text.
  • playground: The bars to expand input/output boxes are gone. They never worked very well. The output one especially has always been broken. But the input one is useful for large inputs and should be restored.
  • playground: While on the playground/ url, the Playground nav goes to a 404 playground/playgound/.
  • nav: Developers scrolls to the text below the "Developers" headline. Better to scroll so you see the headline and know where you are, like it did before.
  • playground: In small screen responsive mode, the playground is still a disaster. :-) Not something to deal with in general for now.
  • playground: In small screen responsive mode, one issue to deal with is the input/options are very narrow by default and not usable, where before it was close to screen width and more usable.
  • Dark mode was surprisingly ok! And other small screen responsive UI was fairly ok too. Some bootstrampisms appear to have fixed older issues. I think it could still use work to look better in places, but that can wait.
  • ... that's all for now on my first pass.

@gkellogg:

I can live without the icons, and it actually gives the site a bit more modern look to not have them.

"modern" is a hard word to interpret in any context. I'd prefer something timeless myself. :-) Not sure why icons are not as popular anymore. Maybe mobile spacing? Maybe hard to find appropriate icons? I did prefer them, and think they are useful for quick visual recognition, and are a hint for those that don't speak our un-internationalized labels. I guess I won't fight everyone on this. Note the screenshot images don't reflect the removed icons.


<link rel="shortcut icon" href="favicon.ico" />

<!-- Script tags -->
<script src="//ajax.googleapis.com/ajax/libs/jquery/1.9.0/jquery.min.js"></script>
<script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/js/bootstrap.bundle.min.js" async></script>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://getbootstrap.com/docs/5.3/getting-started/introduction/#quick-start
Are the quick start docs considered best practice? They use integrity and crossorigin attributes. Is async ok here? It's not in the quick start docs. Does that cause issues with timing and ordering? (Same issue in other files too.)

<div class="col-md-3 gy-3">
<div class="card">
<div class="card-body">
<h3 class="card-title">Javascript</h3>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<h3 class="card-title">Javascript</h3>
<h3 class="card-title">JavaScript</h3>

@@ -1397,12 +1390,13 @@
})
.appendTo(idWrap);

iconCloudDownload.after(" Load");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix indent and use spaces.

@@ -1397,12 +1390,13 @@
})
.appendTo(idWrap);

iconCloudDownload.after(" Load");

const iconCloudUpload = $("<i/>", {"class": "icon icon-cloud-upload"});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix indent and use spaces.

@@ -1411,6 +1405,8 @@
})
.appendTo(descWrap);

iconCloudUpload.after(" Create")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix indent and use spaces.

})
.appendTo(descWrap);

const iconCloudDownload = $("<i/>", {"class": "icon icon-cloud-download"});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix indent and use spaces.

@davidlehn
Copy link
Member

  • playground: Perhaps a compatibility issue with codemirror and newer bootstrap. Load up Event example, use the C-space shortcut, pick something, you get console errors about destroy method, and part of the popup is stuck on the screen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants