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

[Tech debt] Purge the Bootstrap hacks (finally) #405

Open
HebaruSan opened this issue Oct 19, 2021 · 2 comments · May be fixed by #420
Open

[Tech debt] Purge the Bootstrap hacks (finally) #405

HebaruSan opened this issue Oct 19, 2021 · 2 comments · May be fixed by #420
Assignees
Labels
Area: Frontend Related to HTML, JS, CSS, or other browser things Priority: Normal Type: Improvement

Comments

@HebaruSan
Copy link
Contributor

Background

SpaceDock's layout is based on Bootstrap, which has been installed by copying the Bootstrap files to /frontend/static/css and /frontend/static/js.

Problems

Historically, alarmingly, these files were not treated as static dependencies to be left alone and patched with additional overrides from the outside, but as files owned by SpaceDock; edits were made to the Bootstrap files themselves to fix SpaceDock-specific problems or tweak SpaceDock's page layouts.

This is (very?) bad practice because:

  • It's impossible to tell at a glance whether a given style directive is really part of Bootstrap or a hack added later by SpaceDock devs without digging into the git log, which makes investigating and fixing visual problems more difficult.
  • We can't upgrade our version of Bootstrap, which is currently stuck at v3.3.6 (from 2015!!) while the upstream project has moved on to v5.1.3 as I write this, because we would lose all these hack changes if we replaced our static copies of the Bootstrap files, completely breaking the layout.

Suggestions

  • Remove all SpaceDock-specific hacks from these files by checking their git log
    • Move them to separate stylesheet files that are clearly identifiable as SpaceDock-specific
  • Remove the embedded copy of Bootstrap from the source repo
  • Switch to installing Bootstrap with npm
  • Upgrade Bootstrap to a recent version

This would make it much easier for us to make big changes like migrating away from Bootstrap if we wanted to do that, or adding a new frontend dependency like AdminLTE if we wanted to do that (and as @V1TA5 does very much want to do).

@HebaruSan HebaruSan added Type: Improvement Priority: Normal Area: Frontend Related to HTML, JS, CSS, or other browser things labels Oct 19, 2021
@HebaruSan HebaruSan self-assigned this Oct 19, 2021
@HebaruSan
Copy link
Contributor Author

Checking the ancient history of the repo, it looks like this is only a problem for the CSS files, not Javascript. The latter are installed as .min.js files that I do not think have been edited. 🎉

@HebaruSan
Copy link
Contributor Author

HebaruSan commented Oct 29, 2021

Now I'm starting to think I misunderstood what happened. The edits that I was thinking of were for bootstrap-theme.css:

The parts about Bootstrap v3.3.5 and v3.3.6 and copyright Twitter Inc made it sound like this is a standard file from Bootstrap (and indeed, bootstrap does ship a file by that name). Yet the "Generated using the Bootstrap Customizer" part suggests that it wasn't a file owned by the Bootstrap installation itself, but rather that Bootstrap intentionally sets that file aside for sites to customize. So it is OK to edit it, and it's OK to keep it in the repo, which should make it a lot easier to clean up the repo.

/*!
* Bootstrap v3.3.5 (http://getbootstrap.com)
* Copyright 2011-2016 Twitter, Inc.
* Licensed under MIT (https://github.com/twbs/bootstrap/blob/master/LICENSE)
*/
/*!
* Generated using the Bootstrap Customizer (http://getbootstrap.com/customize/?id=a851b0e11cd18449a737c4005160874f)
* Config saved to config.json and https://gist.github.com/a851b0e11cd18449a737c4005160874f
*/
/*!
* Bootstrap v3.3.6 (http://getbootstrap.com)
* Copyright 2011-2015 Twitter, Inc.
* Licensed under MIT (https://github.com/twbs/bootstrap/blob/master/LICENSE)
*/

@HebaruSan HebaruSan linked a pull request Oct 31, 2021 that will close this issue
@HebaruSan HebaruSan linked a pull request Oct 31, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Frontend Related to HTML, JS, CSS, or other browser things Priority: Normal Type: Improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant