-
Notifications
You must be signed in to change notification settings - Fork 2
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
Browse page #387
Browse page #387
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, but at a glance this looks good 👍🏼
style={{ scrollSnapType: 'x mandatory' }} | ||
> | ||
{namespaces ? ( | ||
Object.values(namespaces).map((item, index) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge deal, but a pattern I like over this is:
{namespaces && (
<div> { ... } </div>
)}
Instead of:
{namespaces ? (
<div> { ... } </div>
) : null
}
Almost the same, but the first is a bit cleaner
|
||
return ( | ||
<div className="position-relative"> | ||
{renderLongRow()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not just make this a component? I.e. const LongRow = () => { ... }
import 'bootstrap/dist/css/bootstrap.min.css' | ||
import "bootstrap/dist/js/bootstrap.bundle.min.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? I'm pretty sure I have this on the main.tsx
component...
|
||
export const ProjectAccordion = (props: Props) => { | ||
const { projects } = props; | ||
const [openIndex, setOpenIndex] = useState(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used?
const formatDate = (dateString: string) => { | ||
const date = new Date(dateString); | ||
return date.toLocaleString('en-US', { | ||
year: 'numeric', | ||
month: 'long', | ||
day: 'numeric', | ||
hour: 'numeric', | ||
minute: '2-digit', | ||
hour12: true | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull out into utils?
star.description?.toLowerCase().includes(starSearch.toLowerCase()) || | ||
star.name?.toLowerCase().includes(starSearch.toLowerCase()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
294a2ac
to
9baf03d
Compare
This is a work in progress, but opening this PR to discuss how and whether we should update the routing for the browse page from the current
/schemas
since/schemas/namespace
depends on/schemas
existing..