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

🐛 Fix some code locations that should probably use BASE_URL #858

Closed
wants to merge 7 commits into from

Conversation

nealian
Copy link
Contributor

@nealian nealian commented Aug 19, 2022

nealian Medium ✏️ Draft nealian /base_url → Lissy93/dashy Commits: 7 | Files Changed: 8 | Additions: 5 Label Unchecked Tasks

Thank you for contributing to Dashy! So that your PR can be handled effectively, please populate the following fields (delete sections that are not applicable)

Category:
Bugfix

Overview
A smol change to allow dashy to be fully run in a non-root (URL) context, such as https://example.com/cool-apps/dashy (as opposed to, say, https://dashy.example.com), with all resources being loaded from that context.

Issue Number #857

Code Quality Checklist (Please complete)

  • All changes are backwards compatible
  • All lint checks and tests are passing
  • There are no (new) build warnings or errors
  • (If a new dependency is added) Package is essential, and has been checked out for security or performance
  • (If a new config option is added) Attribute is outlined in the schema and documented
  • Bumps version, if new feature added

@nealian nealian requested a review from Lissy93 as a code owner August 19, 2022 04:41
@netlify
Copy link

netlify bot commented Aug 19, 2022

Deploy Preview for dashy-dev ready!

Name Link
🔨 Latest commit ffca0bd
🔍 Latest deploy log https://app.netlify.com/sites/dashy-dev/deploys/630141d58886ed000a22f72e
😎 Deploy Preview https://deploy-preview-858--dashy-dev.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@viezly
Copy link

viezly bot commented Aug 19, 2022

Changes preview:

Legend:

👀 Review pull request on Viezly

@nealian
Copy link
Contributor Author

nealian commented Aug 19, 2022

Welp, this is only enough to do the job (and then only mostly; the call to save the configuration is still busted, as is a font load) if BASE_URL ends in a /, and I think the loading screen only works there if routingMode is set to hash.
So I'm missing a few things here; I'll come back tomorrow with more (and will more thoroughly test first 😅)
Edit: upon further review of the Vue docs, it is common to require BASE_URL to end in a /, so that is now reflected here.

@nealian nealian marked this pull request as draft August 19, 2022 05:10
@nealian nealian changed the title 🐛 Fix three locations that should probably use BASE_URL 🐛 Fix some code locations that should probably use BASE_URL Aug 20, 2022
@nealian
Copy link
Contributor Author

nealian commented Aug 25, 2022

This has quickly become a spam-fest / game of whack-a-mole / yeah I don't 100% know what I'm doing here -- and I'd rather take the time to figure it out properly. And then squash my commits. Sorry 😅

@nealian nealian closed this Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant