Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React refactor #26

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Conversation

zimengzhou1
Copy link

This refactors the current knoodle frontend into a Next.js app. This makes the code more modular and easier to add improvements on in the future.

@pheyvaer
Copy link
Contributor

Hi @zimengzhou1 Thanks! Can you update the README? Now it seems like it's just a copy of the Next.js default README.

1. Install dependencies via `npm i`.
2. Run production HTTP server via `npm start` (this first runs webpack to generate the browser index.js file), OR
3. Run development HTTP server (that reloads when you make changes) via `npm run watch`
## Local development
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No instructions are present on how the install the Node.js dependencies. I know it's only doing npm i, but it still should be mentioned.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I'll update the readme

noWrap
sx={{ flex: 1 }}
>
Select your pod provider or input custom server
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users should be able to also only provide their WebID. Their pod provider can be determined from that. If there are multiple then a dropbox should be shown picking the desired one.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to have pod providers because I was assuming users are more likely to remember their provider rather than the full webID URL, which they might not remember (at least for me). I could add another text box where users can input their webID.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's best to offer both solution. Mostly because we have no proof at the moment that users prefer one or the other. And even then, the majority (however defined) should not be the only ones considered here.

</>
) : (
<>
<h3>What is this app?</h3>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The app should immediately show the functionality. There is no need to explain the app when opening it. If that is the case then there are problems with the user experience or user interface.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user has to login first anyways, what would you show when the user hasn't logged in yet?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case you show the login options directly. Now people first have to click login at the top and then do the login stuff. This initial click can be avoided by showing the login options directly when launching the app.

You can still have a description of what the app does, but I would remove the header "What is this app?" and rewrite the paragraph to focus on user experience and not what it is from a development standpoint. The fact that it is an alternative to Doodle is kinda irrelevant for the user. What it will actually provide them is what matters.

return (
<SessionProvider restorePreviousSession={true}>
<Head>
<title>Solid Calendar</title>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the app is still "KNoodle".

@renyuneyun
Copy link

Hi. Is there any news on this?
I did some bug fixes, but did not push to the upstream (the "merge from" branch here) as I do not want to mess up the discussion.

@pheyvaer
Copy link
Contributor

pheyvaer commented Dec 1, 2022

Hi @renyuneyun, no, not yet. I think it's better to first clear everything up with the challenge for the back-end then go to the front-end. So you can push your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants