Skip to content

feat(website): add review page#332

Merged
fengelniederhammer merged 1 commit into
mainfrom
273-add-review-page
Oct 17, 2023
Merged

feat(website): add review page#332
fengelniederhammer merged 1 commit into
mainfrom
273-add-review-page

Conversation

@ghost

@ghost ghost commented Oct 5, 2023

Copy link
Copy Markdown
  • use client:only, otherwise everything breaks. No idea why

When using map in react each element should have a unique key. Otherwise react will render everything (in that map) new when a single entry changed. Not fine, but works.
Hypothesis:
But when astro needs to hydrate this components, it cannot associate the correct props and the whole thing goes down.

@ghost ghost linked an issue Oct 5, 2023 that may be closed by this pull request
@netlify

netlify Bot commented Oct 5, 2023

Copy link
Copy Markdown

Deploy Preview for pathoplexus ready!

Name Link
🔨 Latest commit d9950b5
🔍 Latest deploy log https://app.netlify.com/sites/pathoplexus/deploys/652e93a0e7be5000082153e0
😎 Deploy Preview https://deploy-preview-332--pathoplexus.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ghost ghost force-pushed the 273-add-review-page branch 5 times, most recently from 28ab812 to 496d876 Compare October 9, 2023 14:55
@ghost ghost mentioned this pull request Oct 9, 2023
@ghost ghost force-pushed the 273-add-review-page branch 2 times, most recently from 8ca4d50 to dd8b09a Compare October 10, 2023 09:52
@ghost ghost marked this pull request as ready for review October 10, 2023 17:10
@ghost ghost force-pushed the 273-add-review-page branch 2 times, most recently from 1db3867 to 0153a50 Compare October 11, 2023 14:31
@ghost ghost changed the title feat(website): add review page, WIP feat(website): add review page Oct 11, 2023
@chaoran-chen

Copy link
Copy Markdown
Member

use client:only, otherwise everything breaks. No idea why

Are you using MUI? MUI doesn't work properly with Astro's hydration (so I think that we should avoid it whenever there is a decent alternative.)

@ghost ghost marked this pull request as draft October 11, 2023 17:59
@ghost

ghost commented Oct 11, 2023

Copy link
Copy Markdown
Author

only mui icons...

@theosanderson

Copy link
Copy Markdown
Member

Sounds like that's it? withastro/astro#6512

@theosanderson

theosanderson commented Oct 11, 2023

Copy link
Copy Markdown
Member

(There's a possible workaround there if one is wedded to these icons - it also may apply to MUI more generally. I've also used react-icons a lot.)

Comment thread website/README.md Outdated
Comment thread website/src/pages/user/[username]/sequences/[sequenceId]/[version].astro Outdated
Comment thread website/src/pages/user/[username]/sequences/[sequenceId]/[version].astro Outdated
Comment thread website/src/components/Review/ReviewPage.tsx Outdated
Comment thread website/src/components/Review/ReviewPage.tsx Outdated
Comment thread website/src/components/Review/ReviewPage.tsx Outdated
Comment thread website/src/components/Review/ReviewPage.tsx Outdated
Comment thread website/src/components/Review/ReviewPage.tsx Outdated
Comment thread website/src/components/Review/ReviewPage.tsx Outdated
Comment thread website/src/components/Review/ReviewPage.tsx Outdated
@ghost

ghost commented Oct 12, 2023

Copy link
Copy Markdown
Author

download button will be implemented here #358

@ghost

ghost commented Oct 12, 2023

Copy link
Copy Markdown
Author

(There's a possible workaround there if one is wedded to these icons - it also may apply to MUI more generally. I've also used react-icons a lot.)

So, as much changed since the initial commit, now client:load works as expected (even with mui icons). Still a bit curious, what the issue was.

@ghost ghost force-pushed the 273-add-review-page branch from 187713b to e8f161a Compare October 12, 2023 14:39
@ghost

ghost commented Oct 12, 2023

Copy link
Copy Markdown
Author

So, this load vs. only issue occured again and now I know the reason:

When using map in react each element should have a unique key. Otherwise react will render everthing (in that map) new when a single entry changed. Not fine, but works.

Now our hypothesis:
But when astro needs to hydrate this components, it cannot associate the correct props and the whole thing goes down.

@ghost ghost force-pushed the 273-add-review-page branch from a815d8c to cea97c9 Compare October 12, 2023 18:28
@ghost ghost marked this pull request as ready for review October 12, 2023 18:33
@ghost ghost force-pushed the 273-add-review-page branch from cea97c9 to db64232 Compare October 12, 2023 18:35
Comment thread website/src/types.ts Outdated
Comment thread website/src/pages/user/[username]/sequences/[sequenceId]/[version].astro Outdated
@ghost ghost force-pushed the 273-add-review-page branch 2 times, most recently from 2af382e to 0d58413 Compare October 17, 2023 07:49

@fengelniederhammer fengelniederhammer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some nitpicks, some smaller issues, overall it looks good.

Comment thread website/src/components/Review/ReviewPage.tsx Outdated
Comment thread website/src/components/Review/ReviewPage.tsx Outdated
Comment thread website/src/components/Review/ReviewPage.tsx Outdated
Comment thread website/src/components/Review/ReviewPage.tsx Outdated
Comment thread website/src/components/Review/ReviewPage.tsx Outdated
Comment thread website/src/components/Review/DataRow.tsx Outdated
Comment thread website/src/components/Review/ReviewPage.spec.tsx Outdated
Comment thread website/src/types.ts Outdated
Comment thread website/src/pages/user/[username]/review/[sequenceId]/[version].astro Outdated
Comment thread website/src/pages/user/[username]/review/[sequenceId]/getReviewData.ts Outdated

@fengelniederhammer fengelniederhammer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's do the remaining part in a follow up ticket.

Comment thread website/src/components/Review/ReviewPage.tsx Outdated
Comment thread website/src/components/Review/ReviewPage.tsx Outdated
* refactor: prototype for abstracted backend call with proper error handling
@fengelniederhammer fengelniederhammer merged commit ee28e15 into main Oct 17, 2023
@fengelniederhammer fengelniederhammer deleted the 273-add-review-page branch October 17, 2023 14:07
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.

add review page

3 participants