Skip to content
This repository was archived by the owner on Dec 12, 2021. It is now read-only.

Conversation

@rhtaylor
Copy link
Contributor

@rhtaylor rhtaylor commented Oct 31, 2020

This updates the site to have a new division for "Board Members" and "Leadership on the dev team".

A new jsx component is added with additional scss to section the webpage.

image

before
after

  • Local Tests are passing.
  • New tests have been added for new code where relevant
  • Screenshots of before and after changes have been added
  • JSDoc Documentation and comments have been added or updated in the code.
  • Documentation in the /docs folder has been added or updated when appropirate.
  • Follows Nouri JavaScript guidelines
  • Commit message follows Nouri guidelines
  • Introduces minimal or no additional external packages.

Closes #<Issue Number 89>

This is not perfect but just for a little Halloween fun I created a PR. It needs some additional minor scss spacing correction. However works with mobile.

@ambergkim
Copy link
Member

ambergkim commented Nov 1, 2020

Looks like something got broken w/ the mobile first queries:
Screen Shot 2020-11-01 at 1 40 40 PM

EDIT:
Or maybe not. Looks like maybe my browser was confused or needed time.

If my browser is behaving, this will need some repositioning, though
Screen Shot 2020-11-01 at 1 45 44 PM

@ambergkim
Copy link
Member

Can you doublecheck this. The profiles are spilling over the width of the page for me.

Screen Shot 2020-11-01 at 1 58 18 PM

@ambergkim
Copy link
Member

ambergkim commented Nov 5, 2020

@rhtaylor Are you seeing this on your end?

If so, can you get rid of the padding on the left on mobile so that it's more balanced? I don't remember if talked about this one.
Screen Shot 2020-11-04 at 6 16 27 PM

Not seeing the icons vertical on the iPad.
Screen Shot 2020-11-04 at 6 19 43 PM

But am seeing it on iPad horizontal and it's disrupting the flow of everything on the page.
Screen Shot 2020-11-04 at 6 21 25 PM

I think if we're going to have it vertical, it should be vertical for all screens and out of the document flow so that it floats on top and doesn't mess w/ the positioning of everything else. I'm seeing use of flexbox and display block, so it's still in the flow. It would probably be good to move it out into its own component as well.

Otherwise, if removing it from the flow gets too complicated, let's keep it unobtrusive at the bottom. Having it switch it between will get us into trouble w/ screen size changes in between different devices and between device versions.

Document Flow: https://soulandwolf.com.au/blog/what-is-document-flow/
Example of a floating social media bar: https://codepen.io/bhktkshn/pen/YzXawzZ

@ambergkim
Copy link
Member

@rhtaylor Much improved on mobile. 👏

Some more feedback and refinement

Screen Shot 2020-11-08 at 1 06 27 PM

: Let's have the pictures show up on earlier on 1024 width.

Screen Shot 2020-11-08 at 1 05 12 PM

The funkiness at the bottom for larger screens is still overflowing to the right. You can see the border of the top bar ending earlier than the screen. This is probably a matter of the widths of paddings, margins, element widths, and borders, not adding up to more than 100% of the total width.
Screen Shot 2020-11-08 at 1 06 09 PM

We can also work to center the icons on bigger screens.

</a>
</div>
</section>
<BoardMembers />
Copy link
Member

Choose a reason for hiding this comment

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

It would probably better to include this in the section as this whole part can be considered one section:
Screen Shot 2020-11-08 at 1 15 35 PM

The div that encompasses the paragraphs and the pictures can then be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it just me or does anyone else think the pics and words should be centered?

Copy link
Member

Choose a reason for hiding this comment

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

@RaevLogic Are you talking about the text justification or the whole section being off? There is padding that's pushing and making the section off.

Copy link
Contributor

Choose a reason for hiding this comment

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

Untitled
I just edited it in Paint 3D real quick so it's not perfect, but that's more what I was thinking if we're splitting it in two sections like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Two sections being the section with paragraphs and the section with all of our pictures.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only thing I don't like about this from a UI perspective is the split vision aspect. I find it awkward for our SPA to read down as a single column for the majority of the page, and then hit this portion and it's a two-column design where I have to read down the paragraphs and then go back up the page to visually scan back down the pictures section.

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 good with either one or two columns. I think if we have them stacked, I would put the pictures on top and then the text below.

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 we can stick w/ the 2 column for now (unless it gets too annoying for you @rhtaylor ) and then create a new task for 1 column if we want to redesign to that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lol, I'm not exactly a designer over here. Just letting everyone know my thoughts so I'm not wondering if I should have said something for two weeks xD

Copy link
Member

Choose a reason for hiding this comment

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

Constructive feedback is always good. If it can improve user experience, we should think about it.

}
}

.volunteer-content {
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate class name from boardmember.scss

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am thinking this was due to the nesting nature of scss. I believe the class name will have different attributes if it has a different parent so to speak.

Copy link
Member

Choose a reason for hiding this comment

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

Even if they are in different components and React will automate some things once everything has been processed, it's still good practice to be clearer and more intentional about avoiding collisions w/ naming.

@media only screen and (max-width: 430px){
display: none;
}
.volunteer-content{
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate class name from volunteer.scss

Copy link
Member

Choose a reason for hiding this comment

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

For example, on here we are inside the board member scss and not volunteer content. So it's unclear why we are using "volunteer" here.

<button className='btn-container'>volunteer with us</button>
</a>
</div>
<h1 className='volunteer-title'>{SECTION_TITLE}</h1>
Copy link
Member

Choose a reason for hiding this comment

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

Is this class name being used in css anywhere?

Copy link
Contributor Author

@rhtaylor rhtaylor Nov 12, 2020

Choose a reason for hiding this comment

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

I believe this is being used. If not it was intended to be used and was never implemented as I was not on the team when the volunteer sections were created.

Copy link
Member

Choose a reason for hiding this comment

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

No need to keep it if it's not being used.

<div className='main'>
<div className='volunteer-content'>

<h1 className='volunteer-title'>Board Members</h1>
Copy link
Member

Choose a reason for hiding this comment

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

Is this class name being used in css anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you are referring to className 'main' then yes. The others were created before I joined the team.

Copy link
Member

Choose a reason for hiding this comment

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

This one is about the "volunteer-title". Same as above.

margin: auto;
width: 40%;
margin: auto;
padding-left: 20vh;
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 causing the left padding which adds to things being pushed too far to the right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I took care of the extra space in the padding?

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 still causing the issue on my end. Removing it removes the extra padding

With the padding:
Screen Shot 2020-11-14 at 4 16 08 PM

Without:
Screen Shot 2020-11-14 at 4 16 30 PM

Copy link
Member

Choose a reason for hiding this comment

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

If you've removed this, can you double check that you pushed your local changes up. I'm still seeing this here on github

return (
<div>
<>
<div className='links-container'>
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 this links container can actually be put inside the footer tag since it's part of the whole footer. You will have to readjust the styling but the separation between areas of the page will be clearer.

Copy link
Member

Choose a reason for hiding this comment

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

You would also be able to remove the fragment that surrounds the footer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

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 seeing a new span tag added to replace the <> or

instead of all the contents being put under the footer element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I used a span tag. If you want the elements totally I believe react will let me use an array to wrap the elements in [,

] for example. I don't know how well that would work but worth a try.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants