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

Improve error messages #1678

Merged
merged 12 commits into from Oct 2, 2019

Conversation

@luontola
Copy link
Collaborator

commented Oct 1, 2019

Closes #1637

  • show the "Not found" page if the application doesn't exist (or any other ajax request returns 404)
  • resolve flash message text at render time instead of the time when the message was created, to support changing the language or translations which are not yet loaded
  • if loading translations fails, show the translation key as a fallback:
    no translations
  • wait for the translations, themes and config to be loaded before the first render, to avoid a quick flash of an incomplete page

Definition of Done / Review checklist

Reviewability

  • link to issue
  • consider adding screenshots for ease of review
src/cljs/rems/util.cljs Show resolved Hide resolved
@Macroz
Macroz approved these changes Oct 2, 2019
Copy link
Collaborator

left a comment

I'm not against this PR but I wonder if the promises add any value here. Really we should just refactor the three loads away and return them from the backend as data already in the initial page load.

(-> (p/all [(fetch-translations!)
(fetch-theme!)
(config/fetch-config!)])
(p/finally (fn []
(hook-browser-navigation!)
(mount-components)))))
Comment for lines 459  – 464

This comment has been minimized.

Copy link
@Macroz

Macroz Oct 2, 2019

Collaborator

Doesn't this prevent any useful loading spinner from showing up?

How about not fetching any of these here after page load but instead already injecting them into the base page on the server side?

This comment has been minimized.

Copy link
@luontola

luontola Oct 2, 2019

Author Collaborator

The spinner will show up when it's loading the page content, which is what typically takes the longest time, for example in the catalogue page.

This comment has been minimized.

Copy link
@luontola

luontola Oct 2, 2019

Author Collaborator

It's an option to switch to loading everything with ajax or injecting everything to the html. Currently it's a mixture of both. I created an idea ticket for it.

This comment has been minimized.

Copy link
@luontola

luontola Oct 2, 2019

Author Collaborator

The slowest part of the initial page load is downloading and parsing the JavaScript, during which no JavaScript-generated spinner can anyways be shown. It would need to be in the static HTML.

src/cljs/rems/util.cljs Outdated Show resolved Hide resolved
@luontola luontola force-pushed the error-messages branch from 5fa3dd5 to 99d2f1a Oct 2, 2019
@luontola luontola merged commit 619259c into master Oct 2, 2019
7 checks passed
7 checks passed
WIP Ready for review
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: doo Your tests passed on CircleCI!
Details
ci/circleci: ok Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: war Your tests passed on CircleCI!
Details
ci/circleci: without-db Your tests passed on CircleCI!
Details
@luontola luontola deleted the error-messages branch Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.