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

Beautify routes #365

Conversation

carlobeltrame
Copy link
Member

@carlobeltrame carlobeltrame commented Jan 31, 2020

Fixes #345

In this PR...

  • Frontend routes look like /camps/eca44930/SoLa-2020/picasso and /camps/eca44930/He-La-Wolfenstein/events/1a41cdbe/LA-CDs-brennen, but /camps/eca44930 and even /camps/eca44930/picasso and /camps/eca44930/picasso/picasso also work as one would expect
  • the new frontend dependency slugify is added to convert camp titles to URL-safe strings, and its use is centralized in the router
  • The camp and/or event specified in the URL can be injected as props from the router, logic is also centralized there. This is done by traversing the HAL API instead of hardcoding any URI (because we can)
  • The DefaultLayout with lots of v-ifs is split into two separate layouts (DefaultLayout and CampLayout)
  • The API store now outputs more intelligent dummies for arrays that are still loading
  • It is now possible to reload embedded collections, making the gap that was noticed in Embedded Collection vs Link auf Filtered Endpoint #359 smaller
  • Adds convenience links to the home page to seed the database when starting the application for the first time in Docker
  • General clean up of the existing frontend components

pmattmann and others added 28 commits October 5, 2019 12:27
gelöschte Objekte werden mit _meta.deleted markiert
…ster+login

# Conflicts:
#	backend/module/eCampApi/config/module.config.php
#	frontend/src/components/camp/Collaborators.vue
#	frontend/src/store/index.js
#	frontend/src/store/storeValueProxy.js
…ster+login

# Conflicts:
#	backend/composer.lock
#	backend/module/eCampApp.php
#	backend/module/eCampCore/src/Hydrator/CampHydrator.php
#	backend/module/eCampCore/src/Hydrator/PeriodHydrator.php
…spa/register+login

# Conflicts:
#	frontend/package.json
#	frontend/src/views/Home.vue
#	frontend/src/views/auth/Login.vue
#	frontend/vue.config.js
ApiSingleSelect für Change Role implementiert
…ster+login

# Conflicts:
#	frontend/package.json
#	frontend/src/views/auth/Login.vue
#	frontend/vue.config.js
…ster+login

# Conflicts:
#	frontend/src/components/camp/Collaborators.vue
…two separate layouts, add loadingArrayProxy to represent arrays that are currently loading from the API
Copy link
Member

@pmattmann pmattmann left a comment

Choose a reason for hiding this comment

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

Der Store gibt ja Loading-Proxies zurück, solange der eigentliche Inhalt noch unbekannt ist.
Da scheint es noch ein Problem zu geben. Grund habe ich aber noch nicht gefunden.

Das Problem zeigt sich bei einem Browser-Refresh bei der Anzeige eines Events.
http://localhost:8080/camps/6c1216f9/Camp1Title/events/a003ecc7/Event-LA

Wenn direkt die Event-URL abgerufen wird, kann irgendwie nicht alles sauber geladen werden.
Wenn man dahin navigiert, funktioniert es jedoch tip top...

frontend/src/router.js Outdated Show resolved Hide resolved
frontend/src/router.js Outdated Show resolved Hide resolved
The previous solution loaded all camps / all events even if just one is
needed. But hardcoding is also not an optimal solution. The correct
solution according to HAL would be templated relations. This will be
discussed in ecamp#369.
@carlobeltrame
Copy link
Member Author

Habe die diskutierten Punkte umgesetzt. Bitte erneut reviewen.

Copy link
Member

@usu usu left a comment

Choose a reason for hiding this comment

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

Gefällt mir. Sieht gut & clean aus! 👍

Was ich mir noch überlegt hab: Würd es Sinn machen, die URL zu korrigieren, wenn jemand von aussen mit einer alten/falschen URL kommt.
Z.B.
User lädt http://localhost:8080/camps/2881d2da/Alter-Camp-Title

Der Titel wurde in der Zwischenzeit geändert. Router löst natürlich korrekt auf. Sollen wir dann URL umschreiben auf?
http://localhost:8080/camps/2881d2da/Neuer-Camp-Title

Nur so als Idee, keine Ahnung wie aufwändig das ist.

Der Punkt Event vs. EventInstance müssten wir noch klären.

}
},

created: function () {
// force reloading of all events
Copy link
Member

Choose a reason for hiding this comment

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

Nicht glücklich, dass das jetzt wegfällt. Aber ok, diskutieren wir im Issue #359

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, wir könnens auch wieder reinmachen. Hat für mich nach Anti-Pattern und Gebastel ausgesehen, das vom Debugging mal noch übrig geblieben ist, darum hab ichs mal "aufgeräumt".
Mit diesem PR ist es aber ab sofort möglich, sogar embedded collections zu reloaden, falls sich in Zukunft mal jemand unüberlegt ein Beispiel an diesem event reloading nimmt. Wenn ihr das event reloading also explizit (begründet) wollt können wirs auch wieder reinmachen, und vielleicht den duplizierten Code irgendwo zentralisieren.

},

created: function () {
// force reloading of all events
Copy link
Member

Choose a reason for hiding this comment

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

Dito wie oben

}
]
},
{
path: '/camp/:campUri/event/:eventUri',
path: '/camps/:campId/:campTitle?/events/:eventId/:eventName?',
Copy link
Member

Choose a reason for hiding this comment

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

Ich dachte wir wollten eigentlich auf die eventInstance und nicht auf das Event verlinken:

path: '/camps/:campId/:campTitle?/events/:eventInstanceId/:eventName?',

Ansonsten fehlt der Tageskontext für die Seitenansicht.

Copy link
Member Author

Choose a reason for hiding this comment

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

Guter Punkt, dann ändere ich aber auch Event.vue ab und benenne die Variabeln überall eventInstance oder instance statt event.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, agree 👍

@carlobeltrame
Copy link
Member Author

@usu Zum URL korrigieren: Könnte man machen, muss aber via router.push(some_route) geschehen. Ich bin mir nicht ganz sicher was das für Auswirkungen hat wenn die neue Route wieder dieselbe ist. Könnte sein dass der Router alle Komponenten neu macht, alle neu mountet, oder gar nichts (was wir uns wünschen würden). Auf jeden Fall würde die Ladezeit der Seite verlängert, da wir das ganze erst machen können nachdem das Camp von der API geladen ist.
In der aktuellen Implementation hingegen stimmt die URL spätestens wieder nachdem der User irgendeinen Navigations-Link angeklickt hat.

@carlobeltrame
Copy link
Member Author

Habe die Event-Ansicht überarbeitet mit EventInstance-ID in der Route. Dabei habe ich auch gleich die Sidebar gefixt die noch kaputt war, und eine wiederverwendbare Picasso-Komponente erstellt. Wieder bereit zum reviewen :)

It doesn't make sense to test for events() as it's not used here.
Copy link
Member

@manuelmeister manuelmeister left a comment

Choose a reason for hiding this comment

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

Cool!
Ich habe noch herausgefunden, dass es bei der Period-Ansicht jetzt keinen loading-State hat.
Habe das gelöst: https://github.com/manuelmeister/ecamp3/tree/carlobeltrame/beautify-routes-with-configurable-hydrators

@carlobeltrame
Copy link
Member Author

Cool, merci!

@manuelmeister manuelmeister merged commit ae26f47 into ecamp:devel Feb 17, 2020
@carlobeltrame carlobeltrame deleted the beautify-routes-with-configurable-hydrators branch February 17, 2020 18:05
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.

Store-URI in der Route
4 participants