IQBall
/
Application-Web
6
0
Fork 0
Code Issues 49 Pull Requests Packages Projects 5 Releases Wiki Activity
Labels Milestones

Add settings page, refactor session management #118

Merged
maxime.batista merged 6 commits from settings-reborn into master 1 year ago
Conversation 0 Commits 6 Files Changed 17
maxime.batista commented 1 year ago
There is no content yet.
maxime.batista added 4 commits 1 year ago
continuous-integration/drone/push Build is passing Details
dbf6e888cf remove bottstrap
maxime.batista requested review from clement.freville2 1 year ago
maxime.batista requested review from yanis.dahmane-bounoua 1 year ago
maxime.batista requested review from vivien.dufour 1 year ago
maxime.batista requested review from mael.daim 1 year ago
maxime.batista requested review from samuel.berion 1 year ago
maxime.batista added 1 commit 1 year ago
continuous-integration/drone/push Build is passing Details
59ebcf9e71 remove profile picture link verification
clement.freville2 requested changes 1 year ago
index.html Outdated
<script>
var Alert = ReactBootstrap.Alert
</script>
clement.freville2 commented 1 year ago 
-        <script
-            src="https://cdn.jsdelivr.net/npm/react/umd/react.production.min.js"
-            crossorigin></script>
-
-        <script
-            src="https://cdn.jsdelivr.net/npm/react-dom/umd/react-dom.production.min.js"
-            crossorigin></script>
-
-        <script
-            src="https://cdn.jsdelivr.net/npm/react-bootstrap@next/dist/react-bootstrap.min.js"
-            crossorigin></script>
-
-        <script>
-            var Alert = ReactBootstrap.Alert
-        </script>

A build tool is used to avoid that.

```diff - <script - src="https://cdn.jsdelivr.net/npm/react/umd/react.production.min.js" - crossorigin></script> - - <script - src="https://cdn.jsdelivr.net/npm/react-dom/umd/react-dom.production.min.js" - crossorigin></script> - - <script - src="https://cdn.jsdelivr.net/npm/react-bootstrap@next/dist/react-bootstrap.min.js" - crossorigin></script> - - <script> - var Alert = ReactBootstrap.Alert - </script> ``` A build tool is used to avoid that.
maxime.batista marked this conversation as resolved
src/App.tsx Outdated
}
const storedAuth = useMemo(() => getStoredAuthentication(), [])
const fetcher = useMemo(() => new Fetcher(storedAuth), [storedAuth])
clement.freville2 commented 1 year ago 
-    const storedAuth = useMemo(() => getStoredAuthentication(), [])
-    const fetcher = useMemo(() => new Fetcher(storedAuth), [storedAuth])
+    const fetcher = useMemo(() => new Fetcher(getStoredAuthentication()), [])

The stored token is internal to the object, no need to expose it in the rest of the function.

```diff - const storedAuth = useMemo(() => getStoredAuthentication(), []) - const fetcher = useMemo(() => new Fetcher(storedAuth), [storedAuth]) + const fetcher = useMemo(() => new Fetcher(getStoredAuthentication()), []) ``` The stored token is internal to the object, no need to expose it in the rest of the function.
maxime.batista marked this conversation as resolved
src/App.tsx Outdated
fetcher.updateAuthentication(auth)
const user = await fetchUser(fetcher)
setUser(user)
storeAuthentication(auth)
clement.freville2 commented 1 year ago 
-            storeAuthentication(auth)

It is the job of the setUser function to persist in any storage. Don't try to make the storage separated from the state, create a hook that is always synced:

function useSessionState<T>(key: string, initialValue: T): [T, Dispatch<SetStateAction<T>>] {
    const [state, setState] = useState<T>(() => {
        const stored = sessionStorage.getItem(key)
        return stored == null ? initialValue : JSON.parse(stored)
    })
    const setSessionState = useCallback(
        (newState: SetStateAction<T>) => {
            setState((prev) => {
                const value = newState instanceof Function ? newState(prev) : newState
                sessionStorage.setItem(key, JSON.stringify(value))
                return value
            })
        },
        [key],
    )
    return [state, setSessionState]
}
```diff - storeAuthentication(auth) ``` It is the job of the `setUser` function to persist in any storage. Don't try to make the storage separated from the state, create a hook that is always synced: ```ts function useSessionState<T>(key: string, initialValue: T): [T, Dispatch<SetStateAction<T>>] { const [state, setState] = useState<T>(() => { const stored = sessionStorage.getItem(key) return stored == null ? initialValue : JSON.parse(stored) }) const setSessionState = useCallback( (newState: SetStateAction<T>) => { setState((prev) => { const value = newState instanceof Function ? newState(prev) : newState sessionStorage.setItem(key, JSON.stringify(value)) return value }) }, [key], ) return [state, setSessionState] } ```
src/App.tsx Outdated
interface UserContext {
user: User | null
setUser: (user: User) => void
clement.freville2 commented 1 year ago 
-    setUser: (user: User) => void
+    setUser: (user: User | null) => void

The user may want to log out.

```diff - setUser: (user: User) => void + setUser: (user: User | null) => void ``` The user may want to log out.
maxime.batista marked this conversation as resolved
src/App.tsx Outdated
}
const SignedInUserContext = createContext<UserContext | null>(null)
const FetcherContext = createContext(new Fetcher())
clement.freville2 commented 1 year ago

This context is derived from the signed in user. Put them in the same context.

This context is derived from the signed in user. Put them in the same context.
src/app/Fetcher.ts Outdated
response.headers.get("Next-Authorization-Expiration-Date")!,
)
if (nextToken && expirationDate) {
this.auth = { token: nextToken, expirationDate }
clement.freville2 commented 1 year ago 
-        const nextToken = response.headers.get("Next-Authorization")!
-        const expirationDate = new Date(
-            response.headers.get("Next-Authorization-Expiration-Date")!,
-        )
+        const nextToken = response.headers.get("Next-Authorization")
+        const expirationDate = response.headers.get("Next-Authorization-Expiration-Date")
         if (nextToken && expirationDate) {
-            this.auth = { token: nextToken, expirationDate }
+            this.auth = { token: nextToken, expirationDate: new Date(expirationDate) }

If you check the nullability later, using a non-null assertion doesn't make sense.

```diff - const nextToken = response.headers.get("Next-Authorization")! - const expirationDate = new Date( - response.headers.get("Next-Authorization-Expiration-Date")!, - ) + const nextToken = response.headers.get("Next-Authorization") + const expirationDate = response.headers.get("Next-Authorization-Expiration-Date") if (nextToken && expirationDate) { - this.auth = { token: nextToken, expirationDate } + this.auth = { token: nextToken, expirationDate: new Date(expirationDate) } ``` If you check the nullability later, using a non-null assertion doesn't make sense.
maxime.batista marked this conversation as resolved
src/pages/Settings.tsx Outdated
const [errorMessages, setErrorMessages] = useState<string[]>([])
const [success, setSuccess] = useState(false)
const formRef = useRef<HTMLFormElement | null>(null)
clement.freville2 commented 1 year ago

A reference can always be null.

A reference can always be null.
maxime.batista marked this conversation as resolved
src/pages/Settings.tsx Outdated
new FormData(formRef.current!) as Iterable<
[PropertyKey, string]
>,
)
clement.freville2 commented 1 year ago 
-            const { name, email, password, confirmPassword } =
-                Object.fromEntries<string>(
-                    new FormData(formRef.current!) as Iterable<
-                        [PropertyKey, string]
-                    >,
-                )
-
-            if (password !== confirmPassword) {
-                setErrorMessages(["Les mots de passe ne correspondent pas !"])
+            const form = e.target as HTMLFormElement
+            const data = new FormData(form)
+            const name = data.get("name") as string
+            const email = data.get("email") as string
+            const password = data.get("password") as string
+            const confirmPassword = data.get("confirmPassword") as string

Use the target of the event directly.

```diff - const { name, email, password, confirmPassword } = - Object.fromEntries<string>( - new FormData(formRef.current!) as Iterable< - [PropertyKey, string] - >, - ) - - if (password !== confirmPassword) { - setErrorMessages(["Les mots de passe ne correspondent pas !"]) + const form = e.target as HTMLFormElement + const data = new FormData(form) + const name = data.get("name") as string + const email = data.get("email") as string + const password = data.get("password") as string + const confirmPassword = data.get("confirmPassword") as string ``` Use the target of the event directly.
maxime.batista marked this conversation as resolved
src/pages/Settings.tsx Outdated
)
if (password !== confirmPassword) {
setErrorMessages(["Les mots de passe ne correspondent pas !"])
clement.freville2 commented 1 year ago

React offers a way to easily run code as each key stroke, and the browser has a custom validity API. Offer a quick feedback to the user.

const passwordConfirmRef = useRef<HTMLInputElement>(null)
useEffect(() => {
    passwordConfirmRef.current!.setCustomValidity(
        password === confirmPassword ? "" : "Les mots de passe ne correspondent pas",
    )
}, [confirmPassword, password])

When submitting the form, check its validity with checkValidity:

React offers a way to easily run code as each key stroke, and the browser has a custom validity API. Offer a quick feedback to the user. ```ts const passwordConfirmRef = useRef<HTMLInputElement>(null) useEffect(() => { passwordConfirmRef.current!.setCustomValidity( password === confirmPassword ? "" : "Les mots de passe ne correspondent pas", ) }, [confirmPassword, password]) ``` When submitting the form, check its validity with [`checkValidity`](https://developer.mozilla.org/en-US/docs/Web/API/HTMLSelectElement/checkValidity):
maxime.batista marked this conversation as resolved
src/pages/Settings.tsx Outdated
src={profilePicture}
width={width}
height={width}
alt="profile-picture"
clement.freville2 commented 1 year ago 
-                                    alt="profile-picture"
+                                    alt={`Photo de profil de ${user!.name}`}

An ALT text is visible to the end user.

```diff - alt="profile-picture" + alt={`Photo de profil de ${user!.name}`} ``` An ALT text is visible to the end user.
maxime.batista marked this conversation as resolved
src/pages/Settings.tsx Outdated
name="name"
type="text"
placeholder={"Nom d'utilisateur"}
defaultValue={user!.name}
clement.freville2 commented 1 year ago 
-                            <p>Nom d'utilisateur</p>
+                            <label htmlFor="name">Nom d'utilisateur</label>
                             <input
                                 className="settings-input"
                                 name="name"
+                                id="name"
                                 type="text"
-                                placeholder={"Nom d'utilisateur"}
-                                defaultValue={user!.name}
+                                autoComplete="username"
+                                placeholder="Nom d'utilisateur"
+                                required
+                                value={name}
+                                onChange={(e) => setName(e.target.value)}

Labels should be used to bind a legend text with the appropriate input via its ID. The autocomplete attribute allows the browser to sort its suggestions. A required field should have a required attribute. At last, React controlled inputs are great to skip the FormData browser API, and allow custom actions (like checking if the two passwords match easily).

```diff - <p>Nom d'utilisateur</p> + <label htmlFor="name">Nom d'utilisateur</label> <input className="settings-input" name="name" + id="name" type="text" - placeholder={"Nom d'utilisateur"} - defaultValue={user!.name} + autoComplete="username" + placeholder="Nom d'utilisateur" + required + value={name} + onChange={(e) => setName(e.target.value)} ``` Labels should be used to bind a legend text with the appropriate input via its ID. The [autocomplete](https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/autocomplete) attribute allows the browser to sort its suggestions. A required field should have a required attribute. At last, React controlled inputs are great to skip the `FormData` browser API, and allow custom actions (like checking if the two passwords match easily).
maxime.batista marked this conversation as resolved
src/pages/Settings.tsx Outdated
name="email"
type="email"
placeholder={"Addresse email"}
defaultValue={user!.email}
clement.freville2 commented 1 year ago 
-                            <p>Addresse email</p>
+                            <label htmlFor="email">Adresse email</label>
                             <input
                                 className="settings-input"
                                 name="email"
+                                id="email"
                                 type="email"
-                                placeholder={"Addresse email"}
-                                defaultValue={user!.email}
+                                placeholder="Adresse email"
+                                required
+                                value={email}
+                                onChange={(e) => setEmail(e.target.value)

Address takes only one d in French. You may also want to take into account that two users may not share the same email. Your backend currently throws an exception when it is the case.

```diff - <p>Addresse email</p> + <label htmlFor="email">Adresse email</label> <input className="settings-input" name="email" + id="email" type="email" - placeholder={"Addresse email"} - defaultValue={user!.email} + placeholder="Adresse email" + required + value={email} + onChange={(e) => setEmail(e.target.value) ``` *Address* takes only one *d* in French. You may also want to take into account that two users may not share the same email. Your backend currently throws an exception when it is the case.
maxime.batista marked this conversation as resolved
src/pages/Settings.tsx Outdated
className="settings-input"
name="password"
type="password"
placeholder={"Mot de passe"}
clement.freville2 commented 1 year ago 
-                            <p>Mot de passe</p>
+                            <label htmlFor="password">Mot de passe</label>
                             <input
                                 className="settings-input"
                                 name="password"
+                                id="password"
                                 type="password"
-                                placeholder={"Mot de passe"}
+                                autoComplete="new-password"
+                                placeholder="Mot de passe"
+                                value={password}
+                                onChange={(e) => setPassword(e.target.value)}

Help password manager by indicating that the website wants a new password.

```diff - <p>Mot de passe</p> + <label htmlFor="password">Mot de passe</label> <input className="settings-input" name="password" + id="password" type="password" - placeholder={"Mot de passe"} + autoComplete="new-password" + placeholder="Mot de passe" + value={password} + onChange={(e) => setPassword(e.target.value)} ``` Help password manager by indicating that the website wants a new password.
maxime.batista marked this conversation as resolved
src/pages/Settings.tsx Outdated
<button
className="settings-button"
type="submit"
onClick={submitForm}>
clement.freville2 commented 1 year ago 
-                                type="submit"
-                                onClick={submitForm}>
+                                type="submit">
```diff - type="submit" - onClick={submitForm}> + type="submit"> ```
maxime.batista marked this conversation as resolved
src/pages/Settings.tsx Outdated
const fetcher = useAppFetcher()
const [user, setUser] = useUser()
clement.freville2 commented 1 year ago 
+    useEffect(() => {
+        function onKeyUp(e: KeyboardEvent) {
+            if (e.key === "Escape") onHide()
+        }
+        window.addEventListener("keyup", onKeyUp)
+        return () => window.removeEventListener("keyup", onKeyUp)
+    }, [onHide])

The escape key may be used to close a modal as per ARIA patterns.

```diff + useEffect(() => { + function onKeyUp(e: KeyboardEvent) { + if (e.key === "Escape") onHide() + } + window.addEventListener("keyup", onKeyUp) + return () => window.removeEventListener("keyup", onKeyUp) + }, [onHide]) ``` The escape key may be used to close a modal as per [ARIA patterns](https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/).
maxime.batista marked this conversation as resolved
src/pages/Settings.tsx Outdated
if (!show) return <></>
return (
<div id="profile-picture-popup">
clement.freville2 commented 1 year ago 
-        <div id="profile-picture-popup">
+        <dialog id="profile-picture-popup">
```diff - <div id="profile-picture-popup"> + <dialog id="profile-picture-popup"> ```
maxime.batista marked this conversation as resolved
src/pages/Settings.tsx Outdated
: "invalid-input")
}
ref={urlRef}
type="input"
clement.freville2 commented 1 year ago 
-                <p id="profile-picture-popup-subtitle">
-                    Saisissez le lien vers votre nouvelle photo de profil
-                </p>
-                <input
-                    className={
-                        `settings-input ` +
-                        ((errorMessages?.length ?? 0) === 0
-                            ? ""
-                            : "invalid-input")
-                    }
-                    ref={urlRef}
-                    type="input"
-                    placeholder={"lien vers une image"}
-                />

+                    <label id="profile-picture-popup-subtitle" htmlFor="profile-picture">
+                        Saisissez le lien vers votre nouvelle photo de profil
+                    </label>
+                    <input
+                        className={
+                            `settings-input ` +
+                            ((errorMessages?.length ?? 0) === 0
+                                ? ""
+                                : "invalid-input")
+                        }
+                        type="url"
+                        name="url"
+                        id="profile-picture"
+                        required

Use the url input type and a label element.

```diff - <p id="profile-picture-popup-subtitle"> - Saisissez le lien vers votre nouvelle photo de profil - </p> - <input - className={ - `settings-input ` + - ((errorMessages?.length ?? 0) === 0 - ? "" - : "invalid-input") - } - ref={urlRef} - type="input" - placeholder={"lien vers une image"} - /> + <label id="profile-picture-popup-subtitle" htmlFor="profile-picture"> + Saisissez le lien vers votre nouvelle photo de profil + </label> + <input + className={ + `settings-input ` + + ((errorMessages?.length ?? 0) === 0 + ? "" + : "invalid-input") + } + type="url" + name="url" + id="profile-picture" + required ``` Use the `url` input type and a `label` element.
maxime.batista marked this conversation as resolved
src/pages/Settings.tsx Outdated
</button>
<button
className={"settings-button"}
onClick={async () => {
clement.freville2 commented 1 year ago

It is a form, just use a form element and the appropriate event.

It is a form, just use a form element and the appropriate event.
maxime.batista marked this conversation as resolved
src/style/template/header.css Outdated
text-align: center;
vertical-align: center;
margin: 0;
}
clement.freville2 commented 1 year ago 
-#username {
-    text-align: center;
-    vertical-align: center;
-    margin: 0;
-}

No need to change the sign up form.

```diff -#username { - text-align: center; - vertical-align: center; - margin: 0; -} ``` No need to change the sign up form.
maxime.batista marked this conversation as resolved
maxime.batista added 1 commit 1 year ago
continuous-integration/drone/push Build is passing Details
381853b40c apply suggestions
maxime.batista requested review from clement.freville2 1 year ago
clement.freville2 approved these changes 1 year ago
src/pages/Settings.tsx Outdated
name="email"
id="email"
type="email"
placeholder={"Addresse email"}
clement.freville2 commented 1 year ago 
-                                placeholder={"Addresse email"}
+                                placeholder="Adresse email"

Address in English, Adresse in French

```diff - placeholder={"Addresse email"} + placeholder="Adresse email" ``` *Address* in English, *Adresse* in French
maxime.batista marked this conversation as resolved
maxime.batista force-pushed settings-reborn from 381853b40c to 7b4a06a932 1 year ago
maxime.batista force-pushed settings-reborn from 7b4a06a932 to f9c42862e0 1 year ago
maxime.batista merged commit 9e8606184c into master 1 year ago
maxime.batista deleted branch settings-reborn 1 year ago

Reviewers

mael.daim was requested for review 1 year ago
vivien.dufour was requested for review 1 year ago
yanis.dahmane-bounoua was requested for review 1 year ago
samuel.berion was requested for review 1 year ago
clement.freville2 approved these changes 1 year ago
continuous-integration/drone/push Build is passing
Details
The pull request has been merged as 9e8606184c.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
mael.daim
vivien.dufour
yanis.dahmane-bounoua
samuel.berion
clement.freville2
Labels
Apply labels
Clear labels
bug
Something is not working duplicate
This issue or pull request already exists enhancement
New feature help wanted
Need some help invalid
Something is wrong question
More information is needed wontfix
This won't be fixed
No Label bug duplicate enhancement help wanted invalid question wontfix
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: IQBall/Application-Web#118
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'settings-reborn'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes