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

Implement author profiles #1366

Merged
merged 13 commits into from
Jan 26, 2025
Merged

Implement author profiles #1366

merged 13 commits into from
Jan 26, 2025

Conversation

TheOtterlord
Copy link
Member

@TheOtterlord TheOtterlord commented Jan 6, 2025

screenshots

image
image

Browser Test Checklist

I have tested this PR on at least three of the following browsers:

  • Chrome / Chromium
  • Firefox
  • Android Firefox
  • Safari
  • iOS Safari

Copy link

netlify bot commented Jan 6, 2025

Deploy Preview for astro-www-2 ready!

Name Link
🔨 Latest commit 3ce5754
🔍 Latest deploy log https://app.netlify.com/sites/astro-www-2/deploys/679411e7923ee00008b39fad
😎 Deploy Preview https://deploy-preview-1366--astro-www-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
5 paths audited
Performance: 99 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 94 (🟢 up 2 from production)
SEO: 100 (no change from production)
PWA: -
View the detailed breakdown and full score reports

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

@TheOtterlord TheOtterlord marked this pull request as draft January 6, 2025 23:43
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Very cool work @TheOtterlord! Left some notes based on our discussions and also a couple of design thoughts.

src/pages/themes/author/[username]/[...page].astro Outdated Show resolved Hide resolved
src/pages/themes/_components/ThemeCard.astro Outdated Show resolved Hide resolved
src/components/Profile.astro Outdated Show resolved Hide resolved
src/components/Profile.astro Outdated Show resolved Hide resolved
src/components/Profile.astro Outdated Show resolved Hide resolved
src/components/Profile.astro Outdated Show resolved Hide resolved
@TheOtterlord TheOtterlord marked this pull request as ready for review January 16, 2025 15:42
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Looking great @TheOtterlord! And cool to see those articles properly labelled in the accessibility tree:
Chrome dev tools view of a group of article nodes with distinct labels

Spotted a few tiny details but otherwise looking great!

src/components/Profile.astro Outdated Show resolved Hide resolved
src/pages/themes/author/[id]/[...page].astro Outdated Show resolved Hide resolved
src/pages/themes/author/[id]/[...page].astro Outdated Show resolved Hide resolved
@delucis
Copy link
Member

delucis commented Jan 24, 2025

Thanks for the updates @TheOtterlord! Turns out I was wrong about the 404s I guess. It works locally but in the preview deploy I just get a blank page. Guess we should figure out how to do that properly.

One other 404 thing: looks like we need to handle pagination 404s. Currently I get content even if I add an out of range page number e.g. https://deploy-preview-1366--astro-www-2.netlify.app/themes/author/823/

(Edit: ah, I see now it's actually a redirect to page 1. Better than nothing if we can't figure out properly serving a 404)

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

OK, sorry for the churn but recommending we switch back to Astro.redirect() after all 😅

After that, should be good to go I think!

src/pages/themes/author/[id]/[...page].astro Outdated Show resolved Hide resolved
@TheOtterlord TheOtterlord merged commit 47b12d5 into main Jan 26, 2025
6 checks passed
@TheOtterlord TheOtterlord deleted the author-profile branch January 26, 2025 12:46
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.

2 participants