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

Adds people nudge logo to collaborators #3358

Merged
merged 8 commits into from
Sep 13, 2023
Merged

Conversation

kevinjxc
Copy link
Collaborator

@kevinjxc kevinjxc commented Aug 13, 2023

Resolves #3340

Adds if-statement to collaborators section to determine whether to replace LIGA Peatonal with People Nudge.

Things to check before submitting the PR
  • I've written a descriptive PR title.

@kevinjxc
Copy link
Collaborator Author

I couldn't commit the People Nudge logo for some reason, but I was able to place it in public/assets/people_nudge_logo.png.

@misaugstad
Copy link
Member

Can you expand on that?

@kevinjxc
Copy link
Collaborator Author

In the terminal, git status says that the logo is an "untracked change"; when I tried to do git add on the logo, it wouldn't add to the files staged for commit.

@jonfroehlich
Copy link
Member

Would be useful to see before and after screenshots. I really care about how logos are presented on the landing page... I can't tell what this looks like from the PR itself.

@misaugstad
Copy link
Member

Some notes:

  1. As @jonfroehlich said, before/after screenshots should be included. That's what the PR template is for!
  2. With the checkboxes in the PR template, you should replace the space with the "x" instead of just adding the "x". This will make it look like a checked checkbox.
  3. With the issue of not being able to add the logo file, I am still expecting more information. Ideally listing the exact commands you ran, and pasting the output if there is any (or explicitly saying that there was no output if that was the case).

@kevinjxc
Copy link
Collaborator Author

@jonfroehlich Before and after images are attached to this comment.
Screenshot 2023-08-16 at 4 57 09 PM
Screenshot 2023-08-16 at 5 01 48 PM

@misaugstad Tried a bunch of different things and the People Nudge logo should now be committed.

@jonfroehlich
Copy link
Member

Looks good!

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

Just a few things to clean up!

</a>
</div>
@if("Columbus" == cityName){
@if("Taipei" != cityName && "New Taipei" != cityName && "Keelung" != cityName){
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 actually use cityId instead of cityName since the former is always guaranteed to be unique? That's also my bad for using cityName to check for the logos that are specific to Columbus and Amsterdam (could you fix those for me too please? 😁)

</section>
</section>
} else {
<section class="collaborator-images-container">
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 restructure the if/else statement so that we don't duplicate any HTML? You should be able to do something like what we do for Columbus. Any time we can prevent duplicated code, that's a win!

@misaugstad
Copy link
Member

@1kechen now that I've merged your other PR with the alt text for images on the landing page, can you resolve the merge conflicts here and make sure that everything looks good?

@kevinjxc kevinjxc force-pushed the 3340-people-nudge-logo branch from 01e3a37 to 7c488c4 Compare September 9, 2023 22:33
@misaugstad
Copy link
Member

@1kechen it looks like when you dealt with merge conflicts, you must have deleted all your code relating to including the people nudge logo. This is definitely something that I expect you to catch before passing to me for a PR review. After fixing merge conflicts, I expect you to test that the PR still works, which would have revealed the issue. If you go to the "Files changed" tab for the PR, you will also see that your code is gone. Both of those are things I expect you to do before requesting a PR review.

@kevinjxc
Copy link
Collaborator Author

@misaugstad Sorry, I had an error when committing this weekend and the code must've disappeared after I recalled a few previous commits in this branch. I added everything back in and tested it, hope that everything works this time around!

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@misaugstad misaugstad merged commit 62557f3 into develop Sep 13, 2023
@misaugstad misaugstad mentioned this pull request Sep 29, 2023
@misaugstad misaugstad deleted the 3340-people-nudge-logo branch January 11, 2024 19:53
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.

Need People Nudge logo on Taiwan webpage
3 participants