Skip to content

feat: scaffolding for package detail#36

Merged
sdankel merged 6 commits intomasterfrom
sophie/package-detail
Mar 10, 2025
Merged

feat: scaffolding for package detail#36
sdankel merged 6 commits intomasterfrom
sophie/package-detail

Conversation

@sdankel
Copy link
Copy Markdown
Contributor

@sdankel sdankel commented Mar 7, 2025

  • adds and API call to fetch the package metadata, displays it, and sets up the scaffolding for more data to be populated.
  • changes the styles of the app to be dark mode and more fuel-y.
  • ran prettier and added it to CI.

image

image

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
forc-pub ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 10, 2025 8:13pm

@sdankel sdankel marked this pull request as ready for review March 7, 2025 04:12
@sdankel sdankel requested a review from a team March 7, 2025 19:34
@sdankel sdankel enabled auto-merge March 7, 2025 19:34
}, [handleNavigate, logout]);

if (!!user) {
if (user) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just checking if this should be !! to err on the side of caution, or it is safe by the time we get here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They're equivalent. It's only a concern if user can be evaluated as falsy in a way other than null/undefined, such as if user is a string or number.

Copy link
Copy Markdown

@alfiedotwtf alfiedotwtf left a comment

Choose a reason for hiding this comment

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

I can't really comment on the details of the individual React + CSS changes, but nothing jumps out at me. Apart from my question, no reason to not approve from me.

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.

nit: We should try avoid !important since it makes it difficult to identify the heirarchy of CSS overrides

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will address in a future PR

{/* Main Content - Left Side (changes with tabs) */}
<Grid item xs={12} md={7} lg={8}>
{/* Readme Tab */}
{activeTab === 0 && renderReadmeTab()}
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.

nit: rather than conditional rendering based on activeTab index; maybe can put all these rendered components in a list and render using something like tabList[activeTab]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will address in a future PR

Copy link
Copy Markdown
Contributor

@zees-dev zees-dev left a comment

Choose a reason for hiding this comment

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

LGTM 👍

As an improvement I do think we should support both light-mode and dark-mode.
Maybe a future PR.

@sdankel sdankel merged commit 34bb958 into master Mar 10, 2025
9 checks passed
@sdankel sdankel deleted the sophie/package-detail branch March 10, 2025 20:14
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