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

Issue 8/create our projects section #32

Merged
merged 19 commits into from
Nov 6, 2023

Conversation

andycwilliams
Copy link
Member

@andycwilliams andycwilliams commented Oct 27, 2023

This PR:

Resolves #8

Adds both projects sections: a brief one on the main page and more detailed ones on the projects page.

This isn't done and I'm not thrilled with it. But I've stretched myself too thin lately and will be out of town for a couple of days, and it's taken long enough already.

Edit: This PR now only updates the detailed section on the projects page. The brief section is detailed under issue #34 and will receive its own PR.


Screenshots (if applicable):

Capture

Future Steps/PRs Needed to Finish This Work (optional):

  • Clean up and optimize coding (e.g. remove what's unnecessary, abstract when helpful, etc.)
  • Finalize information (i.e. descriptions, tech stacks, etc.)
  • Improve project logo rendering (e.g. make PASS logo transparent like the others, have them all render in a nicer and more uniform way)
  • Populate links in projects page
  • Make sections more responsive and appealing visually

@andycwilliams andycwilliams added the enhancement New feature or request label Oct 27, 2023
@andycwilliams andycwilliams self-assigned this Oct 27, 2023
@Jared-Krajewski
Copy link
Contributor

Jared-Krajewski commented Oct 28, 2023

  1. Can we split each page into a separate pull request. I had not looked at issue Create Projects Page #8 prior to this PR or would have asked for that to be split into two as well.
  2. The rose logo svg will be used so we need that, it will replace the PNG, just waiting on theme to use.
  3. ESBuild windows is in package-lock.json, not sure what you are using it for but it will possibly cause issues for other devs. It's shouldn't be necessary for this project but if it keeps getting added you can remove it from package.json and delete package-lock.json and npm i to re create package.json without it.

@andycwilliams
Copy link
Member Author

This PR should be narrowed to just the Projects page. Can't do a ton of coding at the moment but hopefully this is enough to start testing.

The only change for now is splitting into two branches. All "Futures Steps" that concern this section still apply.

@andycwilliams
Copy link
Member Author

andycwilliams commented Oct 31, 2023

Not fully complete yet. Still a couple of things left to finish (namely rendering the icons and finalizing logos). But should be fairly close and I want to see what everyone thinks so far.

@andycwilliams andycwilliams marked this pull request as ready for review October 31, 2023 22:42
Copy link
Contributor

@Jared-Krajewski Jared-Krajewski left a comment

Choose a reason for hiding this comment

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

needs a fair amount of work still, I can dissect further when I have more time but would like to see some of the commented changes fixed first.

index.html Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
public/assets/roseLogo.svg Outdated Show resolved Hide resolved
{
href: 'https://github.com/codeforpdx/recordexpungPDX',
icon: '<FaGithub size={45} />',
ariaLabel: 'Github link'
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make these all something more specific by just leading with the project name record sponge github link otherwise to screen reader they will all say the same thing regardless of project.

src/components/projects/projectsList.js Outdated Show resolved Hide resolved
src/components/projects/projectsList.js Outdated Show resolved Hide resolved
src/components/projects/projectsList.js Outdated Show resolved Hide resolved
src/components/projects/projectsList.js Outdated Show resolved Hide resolved
src/components/projects/ProjectBox.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@xscottxbrownx xscottxbrownx left a comment

Choose a reason for hiding this comment

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

Besides the commented things...

The file structure in general.

It seems you built the full Projects page in another file (ProjectBox.jsx.)

If want to keep ProjectBox separate, then put the <section> and Header Typography in Projects.jsx. But, I think this filename is misleading.

ProjectBox I read as an individual project's box - as in a single project (a reusable component), so that's why there were multiple ProjectBox's in the Projects.jsx page file. Something like:

projectsList.map(project => {
    <ProjectBox .... />
})

creating a ProjectBox for each project.

It's fine, if you envision it different - but then I would change filename to ProjectsBox or ProjectsSection / but again then there's the debate of whether or not that's just the whole page and code should live in the Projects.jsx file.

Just currently no real purpose to Projects.jsx - it has an empty Typography and just a container wrapping your ProjectBox.jsx. The ProjectBox as is, isn't really a reusable component - it's the whole section.

src/components/projects/ProjectBox.jsx Outdated Show resolved Hide resolved
src/components/projects/ProjectBox.jsx Outdated Show resolved Hide resolved
src/components/projects/ProjectBox.jsx Outdated Show resolved Hide resolved
src/components/projects/ProjectBox.jsx Outdated Show resolved Hide resolved
src/components/projects/ProjectBox.jsx Outdated Show resolved Hide resolved
@andycwilliams
Copy link
Member Author

Made the edits Scott recommended, including making the Chip and Typography more legible as well as semantically consistent.

I also moved the code from ProjectBox directly to the Projects page. I'm of the opinion that since this is the only unique section on the page then there's no real point in separating it. But we can keep discussing. I pushed this change up to show what it looks like this way. Can be changed back.

If we do decide to keep it this way, would we then want to move projectsList.js since it would then be in a folder by itself? It seems unnecessary to have a folder for a single file, but it's very low priority for me.

Next is working on rendering the icons.

@xscottxbrownx
Copy link
Contributor

I will dig into the icons in the array of objects tomorrow!


This looks great though.

The only thing that bothered me was mobile view - the lack of spacing/delineation from the chip to the Overview...

What do you think of this?

Screenshot 2023-11-04 at 7 30 23 PM

which translates to this on large screen:

Screenshot 2023-11-04 at 7 30 13 PM

If liking this ⬆️:

  • uses MUI <Divider /> for line
  • added marginBottom of 1rem to the <Stack> holding the <Chip>
  • the Overview and Technology Used <Typography>'s got mb: 2, mt: 4 instead of just my

@Jared-Krajewski
Copy link
Contributor

Jared-Krajewski commented Nov 5, 2023

To fix the Icons. react is expecting a JSX element not a string.

  1. change projectList.js to projectList.jsx
  2. remove ' from icons in link so they look like
links: [
      {
        href: 'https://github.com/codeforpdx/recordexpungPDX',
        icon: <FaGithub size={45} />
      },
      {
        href: 'https://discord.gg/x6b573et',
        icon: <FaDiscord size={45} />
      },
      {
        href: 'https://codeforpdx.github.io/recordexpungPDX',
        icon: <FaEarthAmericas size={45} />
      }
    ]

on a side note in links.map:
1. react router expects to not href so href={href} becomes to={href}
2. add noreferrer to rel for security
3. I know I commented on changing it before but not sure what I was thinking. No alt is needed as this function is for icons not img

@andycwilliams
Copy link
Member Author

Okay should (hopefully) be very nearly complete. Just need some (hopefully) final feedback. Icons are ready and I did some coding abstraction, which may not even be necessary Logos should look a little nicer now too.

I added a divider, per Scott's suggestion. Looks good but I could could go either way with it. Also, a few commits ago I hid the title on small viewports since it felt redundant. Looks better in my opinion but it can be put back.

Otherwise I did a whole bunch of messing with padding, margins, spacing, and all that. If we agree there's more to be done with that, perhaps we can hold until the next meeting? We're already planning on discussing layout more there, as I understand.

I'm pretty content with this. Let me know what you all think.

@xscottxbrownx
Copy link
Contributor

I added a divider, per Scott's suggestion. Looks good but I could could go either way with it. Also, a few commits ago I hid the title on small viewports since it felt redundant. Looks better in my opinion but it can be put back.

I think for these projects, hiding the title works (could even do it sooner at md vs sm) due to the logos having the titles in them - but that may not be the case for all projects/logos.

Copy link
Contributor

@xscottxbrownx xscottxbrownx left a comment

Choose a reason for hiding this comment

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

In my opinion, we can discuss codestyle throughout full app at meeting (consistency with what to abstract and what not to & what to pull into own file vs leave in parent.)

Think this looks good

Copy link
Contributor

@Jared-Krajewski Jared-Krajewski left a comment

Choose a reason for hiding this comment

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

Talked in discord, pushing changes to merge.

<Typography
variant="body1"
// TODO: Keep textAlign here?
sx={{ textAlign: 'justify' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This weirdly gaps the text and we don't do that anywhere else. Don't see any reason to keep it.

minWidth: { xs: '95vw', sm: '80vw' },
maxWidth: { sm: '85vw' },
background:
index % 2 === 0
Copy link
Contributor

Choose a reason for hiding this comment

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

something you had commented on secondary partners, can we make the gradient go up and down on smaller viewports. If we want to change how the gradient looks after that we can figure out at the meeting but at least it will be consistent for now.

 background: {
    md:
      index % 2 === 0
        ? 'linear-gradient(270deg, rgba(217, 217, 217, 0) 38.54%, rgba(217, 217, 217, 0.4) 82.29%)'
        : 'linear-gradient(90deg, rgba(217, 217, 217, 0) 38.54%, rgba(217, 217, 217, 0.4) 82.29%)',
    xs: 'linear-gradient(0deg, rgba(217, 217, 217, 0) 38.54%, rgba(217, 217, 217, 0.4) 82.29%)'
  }

mb: { xs: 5, sm: 15 },
p: 5,
borderRadius: '25px',
minWidth: { xs: '95vw', sm: '80vw' },
Copy link
Contributor

Choose a reason for hiding this comment

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

This Doesn't constrain within the xl containers and needs to be refactored to cap.

the padding is too much for mobile. This styling fixes these issues.

const boxStyle = (index) => ({
  mb: { xs: 5, sm: 15 },
  py: 5,
  px: 2,
  borderRadius: '25px',
  background: {
    md:
      index % 2 === 0
        ? 'linear-gradient(270deg, rgba(217, 217, 217, 0) 38.54%, rgba(217, 217, 217, 0.4) 82.29%)'
        : 'linear-gradient(90deg, rgba(217, 217, 217, 0) 38.54%, rgba(217, 217, 217, 0.4) 82.29%)',
    xs: 'linear-gradient(0deg, rgba(217, 217, 217, 0) 38.54%, rgba(217, 217, 217, 0.4) 82.29%)'
  }
});

</Container>
<Box
as="section"
sx={{
Copy link
Contributor

Choose a reason for hiding this comment

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

needs to have max size container, currently content can go full width.

Copy link
Contributor

@Jared-Krajewski Jared-Krajewski left a comment

Choose a reason for hiding this comment

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

spoke in discord, pushed fixes for merge.

@Jared-Krajewski Jared-Krajewski merged commit e999d20 into main Nov 6, 2023
@Jared-Krajewski Jared-Krajewski deleted the issue-8/create-our-projects-section branch November 6, 2023 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Projects Page
3 participants