UI tidbits #136
No reviewers
Labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
KittenSquad/glewlwyd!136
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "ui-tidbits"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This is a PR in 5 thematic commits:
Navbar improvements
fa-usericon is used as placeholder if the user has not uploaded a picture.Account selector improvements (Login => Manage logged-in users)
<input>s).Login improvements
fa-userplaceholder is used in the absence of an uploaded profile picture.Cosmetic tweaks (subjective)
@ -42,6 +42,7 @@"error-value-expected": "Waarde vereist","grant-title": "{{- client}} vraagt de volgende toegangsrechten","grant": "Toegang toestaan","grant-none-available": "No grants have been requested",No need to include this one in the Dutch translation, if a i18n isn't found in the selected language, the fallback language (i.e. english) will be used
My review is only technical, because I love the result! Thanks again for the grateful help!
@ -42,6 +42,7 @@"error-value-expected": "Value required","grant-title": "{{- client}} requires access to the following scopes","grant": "Grant access","grant-none-available": "No grants have been requested",I'd say
No requested scope is availableinstead.Unless I'm wrong from the beginning, I use the word 'scope' for the resource requested and the word 'grant' as the authorization a user has granted a client with one or more scopes. Do you agree?
@ -89,10 +89,20 @@ class GrantScope extends Component {infoSomeScopeUnavailable = <div className="alert alert-info" role="alert">{i18next.t("login.info-some-scope-unavailable")}</div>}return ((!Array.isArray(this.state.scope) || this.state.scope.length < 1)I think
Array.isArrayis overkill,this.state.scopeis always supposed to be an array, even an empty one.Also, using a one-liner makes it more difficult to read than a simple
ifAs mentionned in React doc,
stylein react should be avoided, I'd use css insteadWow, that's the kind of pull request I love, thanks a lot @yrammos !
I'll make some technical changes but I won't change a bit of the result.
I was also wondering if having a green button and a red button right next to each other is a good idea, the contrast is very high.
I tried using other bootstrap classes or separating the buttons but I wouldn't say it's better.
I'll try it for a few and I'll probably get used to it. :-)
@ -1,4 +1,4 @@import React, { Component } from 'react';import React, { Component, useState, useEffect } from 'react';import i18next from 'i18next';It looks like
useStateanduseEffectare not used in the component, we should remove them from the import I guess@ -42,6 +42,7 @@"error-value-expected": "Waarde vereist","grant-title": "{{- client}} vraagt de volgende toegangsrechten","grant": "Toegang toestaan","grant-none-available": "No grants have been requested",@babelouest while the line is technically redundant, I inserted it as a trigger for Dutch-speaking contributors to fill-in the missing translation. Your call entirely, of course...
@ -42,6 +42,7 @@"error-value-expected": "Value required","grant-title": "{{- client}} requires access to the following scopes","grant": "Grant access","grant-none-available": "No grants have been requested",Hmm… I'm following you; my only concern with your proposal is it might give the impression of a server-side technical problem, as if some scope (or resource) as such did not exist. Is this indeed the intended message? My understanding is rather that the resource may well exist but the user has no permissions to access it. I might be entirely wrong, though.
@ -89,10 +89,20 @@ class GrantScope extends Component {infoSomeScopeUnavailable = <div className="alert alert-info" role="alert">{i18next.t("login.info-some-scope-unavailable")}</div>}return ((!Array.isArray(this.state.scope) || this.state.scope.length < 1)You know, I vaguely remember having a simple
if (this.state.scope === [])conditional there but it would never be satisfied. I was working under time pressure, though, so I might be wrong. I'd just suggest that you test if you replace with a simpler statement.As for tertiary conditionals, I confess I have a penchant for them. Sorry… :)
Well it should be avoided everywhere. Sorry about that… Bad habit from childhood.
I tried the "warning" style (orange) for logout but it seemed unconvincing. Custom button colors are always a possibility, of course. Thanks for adding padding between the two buttons, as I see in your screenshot.
@ -1,4 +1,4 @@import React, { Component } from 'react';import React, { Component, useState, useEffect } from 'react';import i18next from 'i18next';Apologies — these are leftovers from an abandoned experiment regarding window resize events. They may be safely removed.
(By the way, it is always possible to use the
btn-secondary(gray) class for the "Logout" button, as well. Its privileged position next to "Continue" might bring it out just enough—but not too much. I don't have a strong opinion about this. I'd never wear a green t-shirt with red trousers—or anything with red trousers—but with buttons on a very temporary webpage I can find the contrast pleasing.)@babelouest taking the liberty to add one more (last) commit to this PR. The "New User" link on the Profile page navbar was missing the
&prompt=loginURL parameter, I think.@ -42,6 +42,7 @@"error-value-expected": "Waarde vereist","grant-title": "{{- client}} vraagt de volgende toegangsrechten","grant": "Toegang toestaan","grant-none-available": "No grants have been requested",I know someone who could help us ;)
@ -42,6 +42,7 @@"error-value-expected": "Value required","grant-title": "{{- client}} requires access to the following scopes","grant": "Grant access","grant-none-available": "No grants have been requested",How about
"You don't have access to the requested scopes"?@ -89,10 +89,20 @@ class GrantScope extends Component {infoSomeScopeUnavailable = <div className="alert alert-info" role="alert">{i18next.t("login.info-some-scope-unavailable")}</div>}return ((!Array.isArray(this.state.scope) || this.state.scope.length < 1)I know the feeling, I use them a lot in the backend
@ -89,10 +89,20 @@ class GrantScope extends Component {infoSomeScopeUnavailable = <div className="alert alert-info" role="alert">{i18next.t("login.info-some-scope-unavailable")}</div>}return ((!Array.isArray(this.state.scope) || this.state.scope.length < 1)@yrammos , I realize I already made something similar, but in the App.js
Although, there's only the error message displayed, I think we should add the connected user as well. The error message is in the i18n entry
login.error-scope-unavailable@ -42,6 +42,7 @@"error-value-expected": "Value required","grant-title": "{{- client}} requires access to the following scopes","grant": "Grant access","grant-none-available": "No grants have been requested",That sounds fine to me.
@ -89,10 +89,20 @@ class GrantScope extends Component {infoSomeScopeUnavailable = <div className="alert alert-info" role="alert">{i18next.t("login.info-some-scope-unavailable")}</div>}return ((!Array.isArray(this.state.scope) || this.state.scope.length < 1)@babelouest I don’t think I came across that ,but don’t hesitate to tell me if you’d like me to rewire something.
@ -89,10 +89,20 @@ class GrantScope extends Component {infoSomeScopeUnavailable = <div className="alert alert-info" role="alert">{i18next.t("login.info-some-scope-unavailable")}</div>}return ((!Array.isArray(this.state.scope) || this.state.scope.length < 1)For now, I reset to the original code, I made it specifically when a user is redirected to the login page with a scope list and the user has no scope from the list. Can you retry your tests that lead you to your similar change? I may have forgotten some use cases. If so I'll fix the original code.
done
Hello @yrammos , I did my fix in the branch yrammos-ui-tidbits, can you check that out to verify all your changes are still present?
@ -89,10 +89,20 @@ class GrantScope extends Component {infoSomeScopeUnavailable = <div className="alert alert-info" role="alert">{i18next.t("login.info-some-scope-unavailable")}</div>}return ((!Array.isArray(this.state.scope) || this.state.scope.length < 1)@babelouest ah, I now see what you're referring to. The original code indeed seems to work just as well.
Hello @babelouest, thank you for this! It looks and works very well.
Meanwhile, in #137 you will find a supplementary PR with fixes to bugs that were introduced on
masterbut are best merged intoyrammos-ui-tidbits. Would you kindly review those, as well?@yrammos , let's get back to this PR then, I made some changes in it in my branch yrammos-ui-tidbits, does this branch have all your UI changes in it? If so we can merge it, this will close this PR.
@babelouest your branch works well on my end, except for the following line, where
btn-outline-primaryshould be replaced withbtn-secondary. (The outlined button is from a custom theme that accidentally leaked into the PR.)github.com/babelouest/glewlwyd@c7eedb4c8e/webapp-src/src/Profile/Navbar.js (L200)Also, in retrospect it's not clear to me why the "grants" part of the PR was reverted (the user is still prompted to approve grants (https://github.com/babelouest/glewlwyd/pull/136#discussion_r440429356). Apparently my PR doesn't handle certain scenarios?
Other than these two points, I think we can close this PR and move on.
@yrammos , I fixed the button class you mention in
glewlwyd/webapp-src/src/Profile/Navbar.json line 200.Concerning the grants message I reverted, I suggest we fix it in a separate discussion, can you open an issue to describe the bug and the expected behavior? I believe it will be better off fixed that way.
Meanwhile, I merged this pull request to apply the large amount of improvements it provides.