feat-resources #246
feat-resources #246
Conversation
Harshvardhan1012
force-pushed
the
feat/resources
branch
from
21c9f2f to
ca039ed
Compare
There was a problem hiding this comment.
Praise: @Harshvardhan1012 so great to see you powering through these page updates! This one seems pretty solid, glad that you were able to incorporate the feedback from your last PR.
There was a problem hiding this comment.
Suggestion: Give this a more descriptive name. If it's only for the resources page, possibly include that in the name or put it in a subfolder.
public/images/resources/Vector.png
Outdated
There was a problem hiding this comment.
Suggestion: Give this a more descriptive name. If it's only for the resources page, possibly include that in the name or put it in a subfolder.
There was a problem hiding this comment.
Suggestion: Give this a more descriptive name, especially since it seems reusable.
There was a problem hiding this comment.
Fix: This isn't the right image for the page. Are you just using it as a placeholder?
src/app/resources/assort/page.tsx
Outdated
| mx={{ md: "9", initial: "3" }} | ||
| > | ||
| ASSORT was established as part of a joint partnership between | ||
| <ResourceRedirect text="Boxtribute" link="boxtribute.ord" />,{" "} |
There was a problem hiding this comment.
fix: should be boxtribute.org (not .ord)
src/app/tech/page.tsx
Outdated
There was a problem hiding this comment.
Question: Someone else was working on the tech page. Was there a particular reason to have it use the resources page title? We can create new components but changing components on other pages to consolidate them is more refactoring work and should be tackled in a separate issue.
There was a problem hiding this comment.
In the updated code Page title is not used in any component anymore so I think I can use that as of now
|
|
||
| export const Guide = () => { | ||
| return ( | ||
| <Table.Root size="3" className="bg-gray-50 w-full"> |
There was a problem hiding this comment.
Fix: We should only use tables for tabular data. To keep things simple, can we stray from the design for this and use a
- instead?
| }) => { | ||
| return ( | ||
| <Link | ||
| href={`https://www.${link}`} |
There was a problem hiding this comment.
Question: Are we sure all those sites use the www subdomain? Seems better to just pass through the full href.
src/app/resources/assort/page.tsx
Outdated
| px={{ initial: "3", md: "9" }} | ||
| > | ||
| <ResourcesFooter /> | ||
| <Text> |
There was a problem hiding this comment.
Suggestion: Let's take out this paragraph. We're not really maintaining the "stay up to date" mailing list anymore.
Harshvardhan1012
force-pushed
the
feat/resources
branch
from
ca039ed to
7c9a69a
Compare
What changed?
Resources Page Issue #216
How will this change be visible?