Feature 106: Bug - Fehlerseite bei Suche #114
Conversation
daniel-va
force-pushed
the
feature/asset-106-bug-fehlerseite-bei-suche
branch
from
ee17102 to
ae42ebe
Compare
Replace error page navigation with alerts Move alert feature into `client-shared` library Remove duplicate material styles Fix http response errors being displayed as empty text Fix alert icons getting smaller on long error messages Fix lint errors Fix build issues
daniel-va
force-pushed
the
feature/asset-106-bug-fehlerseite-bei-suche
branch
from
ae42ebe to
5b96eca
Compare
There was a problem hiding this comment.
I like to alert popups, but i think we need to discuss with simon about when to show alerts and when to show the error page and also match the design with boreholes.
Also it does not directly fix the bug of the failed request. I know this is already fixed in another branch, but if we do not managae to finish this other branch in time, then we cannot deploy to production
There was a problem hiding this comment.
We do need some kind of error page, especially for the case were a user is not authorized (does not have the correct group access). We might need to discuss with Simon about which pages errors need to redirect to an error page and which only need to be displayed as pop-ups.
Currently, if a user logs in without the correct permissions, he just sees the loading map and the error popup, but it should then redirect to an error page.
There was a problem hiding this comment.
This has not been discussed with Simon yet - as far as I'm aware, Alerts/Errors in General have not (yet) been standardized across our projects. I will cross-check with Simon if there are any plans to do so in the near future. I assume its no problem to use the design included here for now - we can still change it later with little effort.
In regards to the comment above: This PR directly references the request in #106 to not redirect to an error page when a search fails. Even if the search still errors, this PR solves the expected behavior in the issue.
There was a problem hiding this comment.
I suggest we leave this in (see comment in app.component.ts, so we can display this. We then need a way of handling different kinds of errors to display either the error page or just the popoup alert.
Also, when i initially created the error page, i messed up and it is shifted to the right slightly, this would need to be fixed (margin-left: -80px should do i believe)
There was a problem hiding this comment.
I re-added the error page, but as an overlay instead of a separate page.
| @@ -0,0 +1,7 @@ | |||
| <ng-container *ngFor="let entry of alerts$ | async; trackBy: getAlertId"> | |||
| <li | |||
There was a problem hiding this comment.
Out of curiosity: Why are you doing it like this and not just render the alert component?
There was a problem hiding this comment.
Just for readability, and to separate the loop from its content. It's not strictly better in any way, although starting from v17 Angular supports @for for this exact thing.
There was a problem hiding this comment.
Did you talk to Simon about the design of the alert? also the alert component itself, the icons and animations etc.?
I think before we spend to much time designing, we should check with simon about this, as he also does the design for boreholes, and they should look the same (if they already do, nevermind what i said)
| import { selectAlerts } from '../alert.selectors'; | ||
|
|
||
| @Component({ | ||
| selector: 'ul[app-alert-list]', |
There was a problem hiding this comment.
Same question as above: I have never seen this. What is the benefit over simple selectors?
There was a problem hiding this comment.
When using an element selector (e.g. app-alert-list), we essentially create a new, custom HTML element. This makes sense when there is no existing element that matches the semantic meaning of what we're trying to do. However, if there is such an element, using an attribute selector like here allows us to reuse that element.
This is especially useful in cases like li, which should always follow directly after their parent ul/ol. With attribute selectors, this is possible, e.g. this is valid HTML:
<ul app-alert-list>
<li app-alert-item>My Alert</li>
</ul>While this is not:
<app-alert-list>
<ul>
<app-alert-item>
<li>My Alert</li>
</app-alert-item>
</ul>
</app-alert-list>Finally, attribute selectors also allow us to generate less HTML when we want to wrap a component's content inside a specific HTML element, just like here. Other elements besides li where this is very useful is with things like button, dialog, footer, and more.
| } | ||
|
|
||
| :host.is-removed { | ||
| animation: 250ms ease-out slide-out; |
There was a problem hiding this comment.
Are these animations in accordance with boreholes and Simons design?
There was a problem hiding this comment.
Wouldn't it make sense to have these files in the state folder? (reducer, slector, actions and model?)
There was a problem hiding this comment.
Personally, due to the size of the project and the fact that we might want to move these files in the near future, I think that grouping everything by feature rather than by layer/type makes things a lot easier for us. We can discuss this personally, as I think it would make sense to "officially" decide which way we want go for for now.
daniel-va
force-pushed
the
feature/asset-106-bug-fehlerseite-bei-suche
branch
from
a14ab41 to
34f5234
Compare
| @@ -44,6 +46,20 @@ asset-sg-menu-bar { | |||
| } | |||
| } | |||
|
|
|||
| .error { | |||
There was a problem hiding this comment.
Looks good, but now it is even more aggressive than before ;)
Do you think you could tone it down a bit?
Resolves #106.
Replaces the error page at
/{lang}/errorwith overlay alerts.The search described in #106 is already solved in #102.
It will go live in the search rewrite of #99.