7
0
Fork
You've already forked LibRate
1
forked from mjh/LibRate

WIP: chore: add review comments #3

Closed
Ryuno-Ki wants to merge 1 commit from Ryuno-Ki/LibRate:review-after-the-fact into dev
pull from: Ryuno-Ki/LibRate:review-after-the-fact
merge into: GuildAlpha:dev
GuildAlpha:dev
GuildAlpha:pr3-todos-impl
GuildAlpha:logging-customizations
GuildAlpha:feat-ap-actors
GuildAlpha:fix-db-creation
GuildAlpha:optimize-dockerfile
GuildAlpha:neo4j-support
GuildAlpha:block-llms
GuildAlpha:docker-compose-fixes
GuildAlpha:webfinger
GuildAlpha:docker-compose-setup
GuildAlpha:main
GuildAlpha:fix-svelte-routing
GuildAlpha:no-sveltekit
GuildAlpha:auth-fixes-carousel-improvements
GuildAlpha:film-cards
GuildAlpha:fix-random-media
GuildAlpha:exp
GuildAlpha:federation

I don't have the capacity to contribute to the project myself at the
moment. But I want to highlight areas where I feel a closer look could
be beneficial to the health of the project.

As offered on Matrix I'm adding comments. I haven't checked whether the project builds with these. Please close this PR unmerged once the comments are adressed. (I'm adding WIP: to prevent an accidental merge).

Signed-off-by: André Jaenisch andre.jaenisch@posteo.de

I don't have the capacity to contribute to the project myself at the moment. But I want to highlight areas where I feel a closer look could be beneficial to the health of the project. As offered on Matrix I'm adding comments. I haven't checked whether the project builds with these. Please close this PR unmerged once the comments are adressed. (I'm adding `WIP: ` to prevent an accidental merge). Signed-off-by: André Jaenisch <andre.jaenisch@posteo.de>
I don't have the capacity to contribute to the project myself at the
moment. But I want to highlight areas where I feel a closer look could
be beneficial to the health of the project.
Signed-off-by: André Jaenisch <andre.jaenisch@posteo.de>
@ -64,2 +67,4 @@
event.preventDefault();
// TODO: This nested ternary function is confusing. What is the purpose of
// the Boolean?
Collaborator
Copy link

This is to check if passwords match when the registration flow has been initialized, since the signup form fields are only displayed upon clicking "sign up"

This is to check if passwords match when the registration flow has been initialized, since the signup form fields are only displayed upon clicking "sign up"
@ -89,12 +94,17 @@
if (browser) {
response.ok
// By using localStorage, every JavaScript on the page has access to it
Collaborator
Copy link

Yes that was a temporary hack because I had issues with utilizing svelte stores for global state management between the home page and the auth form component, and I'm well aware of the security implications this has.

Yes that was a temporary hack because I had issues with utilizing svelte stores for global state management between the home page and the auth form component, and I'm well aware of the security implications this has.
@ -95,3 +104,4 @@
authStore.set(data.member),
(authState.id = member.id),
(window.location.href = '/'),
// TODO: Consider using something else than an alert
Collaborator
Copy link

this was also left for debugging, will drop that obviously

this was also left for debugging, will drop that obviously
@ -224,3 +237,4 @@
}
.error-message {
/* TODO: Don't rely on colour alone. Those can be missed by red-green colourblind -->
Collaborator
Copy link

It's also bold. Not sure if alert is a good alternative here, probably not.

It's also bold. Not sure if alert is a good alternative here, probably not.
@ -1,3 +1,5 @@
// TODO: Look into migrating to Rollup v4 at some point:
// https://rollupjs.org/migration/
Collaborator
Copy link

I've updated it with no issues.

I've updated it with no issues.
Collaborator
Copy link

mostly done in upstream. Thank you for your suggestions. Ig I'll make a release after completing some remaining todos.

[mostly done](https://codeberg.org/mjh/LibRate/pulls/49) in upstream. Thank you for your suggestions. Ig I'll make a release after completing some remaining todos.
mjh closed this pull request 2023年11月19日 11:00:13 +01:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
GuildAlpha/LibRate!3
Reference in a new issue
GuildAlpha/LibRate
No description provided.
Delete branch "Ryuno-Ki/LibRate:review-after-the-fact"

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?