Skip to content

Feature/posts#1

Open
joeylaya wants to merge 21 commits intofastersteam:mainlinefrom
joeylaya:feature/posts
Open

Feature/posts#1
joeylaya wants to merge 21 commits intofastersteam:mainlinefrom
joeylaya:feature/posts

Conversation

@joeylaya
Copy link
Copy Markdown
Collaborator

No description provided.

Copy link
Copy Markdown
Collaborator

@cjdeleon62 cjdeleon62 left a comment

Choose a reason for hiding this comment

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

Great first PR! A lot of changes so I'm sorry if I missed anything. Left some questions but feel free to leave any questions you have in any threads or just reach out on discord

const { title, authors, media, publishedOn, text, slug, className } = props;

return (
<article
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

good use of semantic html 👍

};

export const BlogCard: React.FC<Props> = (props) => {
const { title, authors, media, publishedOn, text, slug, className } = props;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

one small nit:
we can destructure this in the function definition:

export const BlogCard: React.FC<Props> = ({ title, authors, media, publishedOn, text, slug, className }) => {

</Link>
<div className="flex flex-col gap-2">
<div className="flex flex-wrap gap-4">
{authors.map(({ id, name, photo }) => (
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

wondering if we can make this its own component to keep files relatively small?

@@ -1,5 +1,5 @@
.richText {
text-align: center; // hack but whatever
// text-align: center; // hack but whatever
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

just wondering if this is something we want to do here? If possible, could we leave it and just override with tailwind css styling?

@@ -0,0 +1,97 @@
import { Fragment } from "react";
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: a cool updated React thing is that we can use <></> instead of <Fragment></Fragment>

Comment thread src/app/layout.tsx
import Link from "next/link";

import "./app.scss";
// import "./app.scss";
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if we don't need this we can def delete it

if (doc?.authors) {
const authorDocs = await Promise.all(
doc.authors.map(
async (author) =>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is there any issues where the author can be something other than an object? Wondering if there's a way we can ensure that it's always an object

import { checkRole } from "../collections/Users/checkRole";

export const adminsAndPublished: Access = ({ req: { user } }) => {
if (user && checkRole(["admin"], user)) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

would we be able to reuse the isAdmin function here?


export const adminsAndSelf: Access = ({ req: { user } }) => {
if (user) {
if (checkRole(["admin"], user)) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same question about this using isAdmin

): boolean => {
if (user) {
if (
allRoles.some((role) => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit:
what do you think about using Array.prototype.every() here?

const matchesAllRoles = allRoles.every((role) => user?.roles?.includes(role));

return matchesAllRoles;

@@ -0,0 +1,14 @@
export const formatDateTime = (timestamp: string): string => {
const now = new Date();
let date = now;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do we need the const now = new Date()?

could we just do let date = new Date();

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