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

Generate static urls #11571

Open
wants to merge 15 commits into
base: dev/8.0.x
Choose a base branch
from
Open

Conversation

chrabyrd
Copy link
Contributor

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

This removes dependency on arches_urls.htm for all Vue frontend work.

Issues Solved

Closes #11567

Checklist

  • I targeted one of these branches:
    • dev/8.0.x (under development): features, bugfixes not covered below
    • dev/7.6.x (main support): regressions, crashing bugs, security issues, major bugs in new features
    • dev/6.2.x (extended support): major security issues, data loss issues
  • I added a changelog in arches/releases
  • I submitted a PR to arches-docs (if appropriate)
  • Unit tests pass locally with my changes
  • I added tests that prove my fix is effective or that my feature works
  • My test fails on the target branch

Accessibility Checklist

Developer Guide

Topic Changed Retested
Color contrast
Form fields
Headings
Links
Keyboard
Responsive Design
HTML validation
Screen reader

Ticket Background

  • Sponsored by:
  • Found by: @
  • Tested by: @
  • Designed by: @

Further comments

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Principle makes sense. Noticed one behavior change I think we should look into. Will kick the tires again after.

@@ -22,7 +22,7 @@
<!DOCTYPE html>
<!--[if IE 8]> <html lang="en" class="ie8"> <![endif]-->
<!--[if IE 9]> <html lang="en" class="ie9"> <![endif]-->
<!--[if !IE]><!--> <html lang="en"> <!--<![endif]-->
<!--[if !IE]><!--> <html lang="{{ app_settings.ACTIVE_LANGUAGE }}"> <!--<![endif]-->
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this more since yesterday, this might be alright after all, given the current pattern of using the i18n pattern routes from Django. On language switch, reload the page, see the updated /lang/... route. Hash out a different pattern if/when we need it.

arches/app/templates/base.htm Show resolved Hide resolved


def _generate_urls_json():
def generate_human_readable_urls(patterns, prefix="", namespace="", result={}):
Copy link
Member

Choose a reason for hiding this comment

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

I try to avoid mutable default arguments like [] and {}, although I do see you've wrapped this all in a closure. Just wondering if you'd prefer to make this a little more explicit so someone else isn't surprised that result persists between calls.

Comment on lines +75 to +77
"OPTIONS": {
"options": "-c cursor_tuple_fraction=1",
},
Copy link
Member

Choose a reason for hiding this comment

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

I had this intended like that for easier copy-pasting, since it's two levels deep. Maybe it's better to keep the indenting and clarify in the verbiage that this is all underneath "default:".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh no fair enough, I just ran the doc through a formatter and that popped out. Happy to revert


1. Replace the `generate_frontend_configuration` import statement in `apps.py`:
```
from arches.settings_utils import generate_frontend_configuration
Copy link
Member

Choose a reason for hiding this comment

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

I toyed with importing this into the old location so that projects wouldn't need to update, but things broke, so 👍

@@ -57,6 +58,8 @@ JavaScript:

- `ensure_userprofile_exists()` was removed from the `Tile` model.

- The `arches` object is no longer available in frontend components that exist outside of defined Arches patterns. If your project contains Vue components that are mounted on templates that inherit directly from `base-root.htm`, they will need to be updated to use `generateArchesURL` and the settings API instead.
Copy link
Member

Choose a reason for hiding this comment

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

Suggest something a little more specific than "defined patterns", which makes sense to people who read on about base-root.htm and know what that is, but others might need help understanding if this applies to them.

Comment on lines +36 to +37
def join_paths(*args):
return "/".join(filter(None, (arg.strip("/") for arg in args)))
Copy link
Member

Choose a reason for hiding this comment

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

We seem to be losing the trailing slash on some routes. Leaving aside whether it's a good idea to have trailing slashes required in your routes, if projects do have that, I think we should leave it to them (and whatever third-party packages even outside of arches they depend on).

For instance, with status quo, we have on javascript.htm:

reorder_cards="/reorder_cards/"

but now in urls.json:

    "reorder_cards": "/reorder_cards",

giving:

> _arches_utils_generate_arches_url_ts__WEBPACK_IMPORTED_MODULE_11__["default"]('reorder_cards')
'/reorder_cards'

Which will cause a redirect (or not) depending on the value of APPEND_SLASH and yada yada.

Can we avoid a behavior change here?

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.

Generate static urls for frontend consumption
3 participants