enso-ui / auth

ui authentication components
MIT License
0 stars 2 forks source link

Login component rendered after login on homepage #401 - avoid setting `intendedRoute` to 'login' #6

Closed robbykrlos closed 3 years ago

robbykrlos commented 3 years ago

@Reviewer :

  1. I've tested similar behavior for intendedPath and did not find the same issue. If after session is expired the URL is force to a page where filters were loaded incorrectly before table, the intendedPath is not set to "/login". Therefore, I did not felt that setIntendedPath needs changes.
  2. Regarding the explicit function codding - I've seed that you do not normally use hard to read code like:
    setIntendedRoute: (state, value) => (value && value.name !== 'login' && (state.intendedRoute = value))

    neither one-line-ifs, so I keep the code plain and simple, similar to other enso-ui implementations.

Feel free to choose whatever form you like 👍🏼

aocneanu commented 3 years ago

@robbykrlos I look at this and somehow I feel that it would be better to have this check here

what do you think?

robbykrlos commented 3 years ago

@robbykrlos I look at this and somehow I feel that it would be better to have this check here

what do you think?

@aocneanu it felt a bit wrong since intendedPath did not have any issues, so it was something more specific to intendedRoute whet it was set as login route specifically.

So yes, seems better this way, and we'll keep the setter nice and simple.

Let me give it some tests and will PR this new change