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

Feature - Resources admin dashboard #106

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

tfredh
Copy link

@tfredh tfredh commented Oct 19, 2022

No description provided.

@tfredh
Copy link
Author

tfredh commented Oct 19, 2022

Uhh, looks like my first PR's branch's commits were combined with this branch

@tfredh tfredh requested a review from xsharonhe October 19, 2022 23:15
@tfredh
Copy link
Author

tfredh commented Oct 25, 2022

lemme know about any structuring problems irrelevant of size. I just kinda autopiloted for a couple hours in these 5 days XD

@tfredh
Copy link
Author

tfredh commented Oct 25, 2022

had this ready to copy paste from my personal utility code dump

@tfredh
Copy link
Author

tfredh commented Oct 27, 2022

good part of this was also from my code dump so it didn't take very long

@xsharonhe
Copy link
Member

Uhh, looks like my first PR's branch's commits were combined with this branch

next time: https://stackoverflow.com/questions/36168839/how-to-remove-commits-from-a-pull-request

Copy link
Member

@xsharonhe xsharonhe left a comment

Choose a reason for hiding this comment

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

I'll review more later but if you could show a demo of all this, it would be great. Kinda hard to follow all the files without this since I don't know how it is to be used as intended. Good work so far though :D

Comment on lines 3 to 5
// function HorizontalSeparator() {
// return <></>;
// }
Copy link
Member

Choose a reason for hiding this comment

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

cleanup - remove this?

@@ -20,7 +20,8 @@ export const Input = ({
/>
);

const SInput = styled.input`
// used when ref needs to be passed as prop
export const SInput = styled.input`
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this doesn't work with just the Input? I think if you want a ref as a prop you can make it on the original element and pass it as optional?

import { useCallback, useState } from "react";
import "easymde/dist/easymde.min.css";

export default function MDEditor({
Copy link
Member

Choose a reason for hiding this comment

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

Can you record a GIF demo for this?

Comment on lines 15 to 17
<div className="modified-time-stamp">
Last changed {latestTimeAgo(blog.lastModified)} ago
</div>
Copy link
Member

Choose a reason for hiding this comment

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

I would be very careful about className - the purpose of styled components is to remove the need for class names so there isn't duplicates throughout our codebase. I think here might be fine since it is a very specific component but just keep this in mind

Comment on lines 12 to 14
<a className="title" href={`/resources/admin/project/blog/${blog.id}`}>
{blog.name}
</a>
Copy link
Member

Choose a reason for hiding this comment

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

like I think here you could get away with a nested link instead of className

Comment on lines 3 to 4
const HEADER_COLOUR = "#1e2530";
const HEADER_HEIGHT = "6vh";
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be better to abstract this into a file in the util section instead of creating a new file for resourcesAdmin - maybe utils/admin?

Copy link
Member

Choose a reason for hiding this comment

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

Are there anything in common with these components? I think they should be likely broken down into separate files because an external developer would have no idea which components are in which files.

Typically components go in the components file and then you can probably create an admin subfile from that. The const colours and height should go in the theme/theme.js file


// attempt to log user in right after page load
useEffect(() => {
async function f() {
Copy link
Member

Choose a reason for hiding this comment

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

a little weird naming? if this is temporary probably fine

lib/envs.js Outdated
Comment on lines 1 to 3
export const ENVS = {
a: 1,
};
Copy link
Member

Choose a reason for hiding this comment

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

this is...? 😅

Copy link
Author

@tfredh tfredh Oct 28, 2022

Choose a reason for hiding this comment

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

o XD I was making mistakes with env variables so I was going to just store them all here and check for attributes. after that I would import it if I ever needed env variables

Comment on lines 7 to 11
/**
* asdjkh
*
* stuff
*/
Copy link
Member

Choose a reason for hiding this comment

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

remove comment here

@tfredh
Copy link
Author

tfredh commented Oct 28, 2022

yeah np will do demo soon

@tfredh
Copy link
Author

tfredh commented Jun 10, 2023

Small things

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