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

Join us page #1002

Merged
merged 25 commits into from
Mar 17, 2021
Merged

Join us page #1002

merged 25 commits into from
Mar 17, 2021

Conversation

trtincher
Copy link
Contributor

@trtincher trtincher commented Feb 4, 2021

This is the new join us page. Access with "localhost:4000/join-us".
This corresponds to issue: #752 (this does not close the issue)
All layouts and styling are in compliance with the Figma

Desktop

Screen Shot 2021-02-04 at 11 03 58 AM
Screen Shot 2021-02-04 at 11 04 09 AM
Screen Shot 2021-02-04 at 11 04 20 AM

Mobile

Screen Shot 2021-02-04 at 11 04 57 AM
Screen Shot 2021-02-04 at 11 05 10 AM
Screen Shot 2021-02-04 at 11 05 20 AM

Copy link
Member

@dmcneary dmcneary left a comment

Choose a reason for hiding this comment

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

Mobile view looks good! I see a couple of typos:

  • h1 text in the first card ('Advice' vs 'Advise')
  • some typos in that card's body text
  • top/bottom margins between the buttons in the third card (Partner with us) narrower compared to Figma mock
  • top/bottom margins between cards are narrower compared to Figma mock
  • 'hero' element in desktop view could probably be moved up towards the main nav a bit

Copy link
Member

@daniellex0 daniellex0 left a comment

Choose a reason for hiding this comment

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

This page is looking great, it's really close! I added my notes about changes in the Figma file, to the right of the notes from before: https://www.figma.com/file/0RRPy1Ph7HafI3qOITg0Mr/Hack-for-LA-Website?node-id=2991%3A0

@akibrhast
Copy link
Member

akibrhast commented Feb 22, 2021

@trtincher Couple of things I have noticed:

  1. Almost all the new files created under _credits in this commit has spaces in their filename. Maybe do something like this for example: _credits/building-2.md . Since that would conform with the current naming scheme within the _credits folder as well as the entire repo.
  2. The changes made to the file _data/navigation/main.yml seems to be something that was automatically done by ide/editor. Should it be removed from the commit ?
  3. The value of the artist key in all the new md files created does not conform same casing style. Some are camel case some are lower case. Shouldn't they all conform to the same format/casing style ?

Also if someone could please explain to me why this file _sass/main.scss is showing to have a merge conflict .. When it looks like @trtincher has just added a new line to it ... ?

@daniellex0 @dmcneary

@trtincher trtincher requested a review from daniellex0 February 24, 2021 03:23
@trtincher
Copy link
Contributor Author

trtincher commented Feb 24, 2021

Reviewed this in Dev meeting on 02/23/2021 and there where duplicate files. Will remove these files and re-commit.

@daniellex0
Copy link
Member

@trtincher The page looks perfect! 🙌

Just one tiny edit:
adding margin-left: 48px
to (nested):
@media (min-width: 768px) .join-us-card-container .page-card .join-us-donation

And then it should be good to go! Thanks for all of the work on this!

@@ -7,10 +7,10 @@

- name: Press
link: /#press

Copy link
Member

Choose a reason for hiding this comment

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

There does not seem to have been any change to this file in regards to this pull request. Any chance this could be removed from the pull request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know the git command to do that?

Copy link
Member

Choose a reason for hiding this comment

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

Not off the top of my head, no. You will have to look into that. But i believe this might be it from the contrrib.md file(Please do double check tough).

-Use the git reset HEAD command to remove a staged file.

This command will remove a file that has been staged. This file will not be committed (saved) when you run the next command, git commit. This only works if the wrong files were added, but they were not yet committed. The file will be removed from the staging area, but not actually deleted:

git reset HEAD “filename.ext”

alt="join us card image"
/>
<div class="join-us-card-body">
<p class='join-us-remove-p-padding'>
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible for the content of this paragraph to be inside a liquid for loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly possible. I see what you mean. Could be a good refactor

@akibrhast
Copy link
Member

We are waiting for @martham0 to nest her classes #1154 and then this will be re-reviewed

@akibrhast akibrhast marked this pull request as draft March 3, 2021 03:36
@akibrhast akibrhast marked this pull request as ready for review March 10, 2021 03:55
@akibrhast
Copy link
Member

@trtincher , @martham0 #1154 has now been merged

Copy link
Member

@daniellex0 daniellex0 left a comment

Choose a reason for hiding this comment

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

If everything looks good after Martha fixed her page card class that was affecting this page, then this is approved 👍

Copy link
Member

@jbubar jbubar left a comment

Choose a reason for hiding this comment

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

few minor request! this looks great!
I would be curious to hear what @daniellex0 has to say about my requests.

join-us.html Outdated
<a href="https://www.linkedin.com/in/bonnieawolfe/">Bonnie Wolfe</a>
</li>
<li>
<a href="[email protected]">Send and email</a> to our
Copy link
Member

Choose a reason for hiding this comment

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

needs to be href="mailto:[email protected]" otherwise it wont work..
Like line 149

join-us.html Outdated
<li>Join our slack (if you are comfortable using slack)</li>
<li>
Connect with out Executive Director,
<a href="https://www.linkedin.com/in/bonnieawolfe/">Bonnie Wolfe</a>
Copy link
Member

Choose a reason for hiding this comment

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

could you add target="_blank" attribute to all of the a tags so that they all open in a new tab?

Copy link
Member

Choose a reason for hiding this comment

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

@jbubar Yes, good call 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think I should add it to these @jbubar @daniellex0 ? Seems a bit excessive to me in this instance.
Screen Shot 2021-03-16 at 8 01 09 PM

join-us.html Outdated
</p>
<p class='join-us-remove-p-padding'>How to get involved:</p>
<ol>
<li>Join our slack (if you are comfortable using slack)</li>
Copy link
Member

Choose a reason for hiding this comment

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

Im not a designer.. but is it possible to provide a link to our slack here?

Copy link
Member

@daniellex0 daniellex0 Mar 15, 2021

Choose a reason for hiding this comment

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

@jbubar You are totally right, thank you for catching this- here is the slack self-invite link @trtincher :

https://hackforla-slack.herokuapp.com/

(Make the word "slack" the link in "Join our slack")

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@Lol-Whut Lol-Whut merged commit a362fda into hackforla:gh-pages Mar 17, 2021
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.

6 participants