application-research / delta-ui

7 stars 0 forks source link

feat: use page routing and move state into context #78

Closed elijaharita closed 1 year ago

elijaharita commented 1 year ago

This is quite a vast diff.

Simultaneously moves state management into a shared context, and moves pages into the app directory to enable routing. These were co-dependent changes so I did them both at the same time.

This is my first time working with app directory and context. Everything works but lmk if I did something obviously strangely.

There is 1 regression: the loading bar doesn't show while fetching from DDM. This requires a bit of extra work, I'll open an extra issue to resolve this.

Fixes #4 Fixes #54

jcace commented 1 year ago

I'm not able to build it for some reason:

delta-ui@0.2.0 build
next build

warn - You have enabled experimental feature (appDir) in next.config.js.
warn - Experimental features are not covered by semver, and may cause unexpected or broken application behavior. Use at your own risk.
info - Thank you for testing appDir please leave your feedback at https://nextjs.link/app-feedback

info - Creating an optimized production build
info - Compiled successfully
info - Linting and checking validity of types ..Failed to compile.

Type error: Page "app/ddm/page.tsx" does not match the required types of a Next.js Page.
Expected "ReactNode | Promise<ReactNode>", got "void".
elijaharita commented 1 year ago

I'm not able to build it for some reason:

delta-ui@0.2.0 build
next build

warn - You have enabled experimental feature (appDir) in next.config.js.
warn - Experimental features are not covered by semver, and may cause unexpected or broken application behavior. Use at your own risk.
info - Thank you for testing appDir please leave your feedback at https://nextjs.link/app-feedback

info - Creating an optimized production build
info - Compiled successfully
info - Linting and checking validity of types ..Failed to compile.

Type error: Page "app/ddm/page.tsx" does not match the required types of a Next.js Page.
Expected "ReactNode | Promise<ReactNode>", got "void".

thanks for catching this, i didn't notice it since i only tested the dev build. i'll make sure to do a full build before opening PRs in the future.

the problem was from not returning a value in the body. redirect returns the never type so i put a return statement on the function call and now build works properly.

jcace commented 1 year ago

@elijaharita for some reason it's not prompting me for the DDM API URL any more when the page loads, even though I tried clearing localstorage and cookies. So it's failing to load as it's trying to go to localhost (I'm testing it on my staging server so I need to set it to the server's IP)

Screenshot 2023-07-03 at 12 40 49 PM
elijaharita commented 1 year ago

@elijaharita for some reason it's not prompting me for the DDM API URL any more when the page loads, even though I tried clearing localstorage and cookies. So it's failing to load as it's trying to go to localhost (I'm testing it on my staging server so I need to set it to the server's IP)

Screenshot 2023-07-03 at 12 40 49 PM

hmmm interesting. i think i might know what's happening. let me check

elijaharita commented 1 year ago

@jcace it turned out connection error wasn't being handled when checking auth, only listening for 401. it might not even be a regression, i wonder if it's always been an issue that just hadn't come up. anyway, i attempted a fix, let me know how it goes.

jcace commented 1 year ago

@jcace it turned out connection error wasn't being handled when checking auth, only listening for 401. it might not even be a regression, i wonder if it's always been an issue that just hadn't come up. anyway, i attempted a fix, let me know how it goes.

Yup, it's working now - only thing is this is being rendered inside the window so you can still mess it up by clicking into the sidebar when the url/token is not specified. Any way to make the sidebar disabled or have this as a separate /setup route that you can't navigate away from until you provide this info?

Screenshot 2023-07-03 at 1 18 18 PM
jcace commented 1 year ago

Also - fine if we wanna do in a separate task - but after modifying stuff (ex, adding a replication, managing a SP), we should refresh the page that it's currently on to reload and show the changes

jcace commented 1 year ago
Screenshot 2023-07-03 at 6 24 06 PM

The Attach Content button under each dataset doesnt appear to do anything now

jcace commented 1 year ago

re: Loading bar - I only noticed it was missing on the Wallets page. Let me know once you've created an issue and I'll track 👍

elijaharita commented 1 year ago
Screenshot 2023-07-03 at 6 24 06 PM

The Attach Content button under each dataset doesnt appear to do anything now

fixed! i accidentally had two separate states, one in the DDM context, and the other in the ddm layout as a useState. i was able to consolidate them by overriding the default state i generate in ddm.ts before passing it into the provider. let me know if it's working properly for you.

elijaharita commented 1 year ago

re: Loading bar - I only noticed it was missing on the Wallets page. Let me know once you've created an issue and I'll track 👍

i actually fixed this already in another PR that depends on this one. it's here - #79

elijaharita commented 1 year ago

Also - fine if we wanna do in a separate task - but after modifying stuff (ex, adding a replication, managing a SP), we should refresh the page that it's currently on to reload and show the changes

oops. all the pages already did this but editing providers was missing a call to refresh. fixed.

elijaharita commented 1 year ago

@jcace it turned out connection error wasn't being handled when checking auth, only listening for 401. it might not even be a regression, i wonder if it's always been an issue that just hadn't come up. anyway, i attempted a fix, let me know how it goes.

Yup, it's working now - only thing is this is being rendered inside the window so you can still mess it up by clicking into the sidebar when the url/token is not specified. Any way to make the sidebar disabled or have this as a separate /setup route that you can't navigate away from until you provide this info?

Screenshot 2023-07-03 at 1 18 18 PM

i added a blurred backdrop to prevent users from clicking. aside from that, i also made it so you will always get redirected back to auth when there's an auth error anyway.