Skip to content

Updated title-link for error in the credits pages#3864

Merged
arpitapandya merged 1 commit intohackforla:gh-pagesfrom
steven-positive-tran:credit-fix-link-3823
Jan 26, 2023
Merged

Updated title-link for error in the credits pages#3864
arpitapandya merged 1 commit intohackforla:gh-pagesfrom
steven-positive-tran:credit-fix-link-3823

Conversation

@steven-positive-tran
Copy link
Member

@steven-positive-tran steven-positive-tran commented Jan 24, 2023

Fixes #3823

What changes did you make and why did you make them ?

  • Updated title-link in credit/404.yml

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

No visual change in the website

@github-actions
Copy link

Want to review this pull request? Take a look at this documentation for a step by step guide!

From your project repository, check out a new branch and test the changes.

git checkout -b steven-positive-tran-credit-fix-link-3823 gh-pages
git pull https://github.com/steven-positive-tran/website.git credit-fix-link-3823

@github-actions github-actions bot added good first issue Good for newcomers P-Feature: Credit https://www.hackforla.org/credits/ role: back end/devOps Tasks for back-end developers role: front end Tasks for front end developers size: 1pt Can be done in 4-6 hours labels Jan 24, 2023
@Adastros Adastros self-requested a review January 24, 2023 05:33
@Adastros
Copy link
Member

Availability (PST): 9:30 - 11PM 23JAN23
Review Estimate: 10:30PM 23JAN23

Copy link
Member

@Adastros Adastros left a comment

Choose a reason for hiding this comment

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

Hi @steven-positive-tran, good job on completing the issue! The correct link was added and works when clicked on the credits page.

I noticed two things that should be fixed.

  1. In issue #3823 the task list (check boxes) are not checked off. They should be checked as you're completing the issue to help indicate what has been completed for a issue.
  2. The issue was moved to the Questions / In Review column. I believe this should be moved back to the In progress column of the project board. The Questions / In Review column is mainly for situations where you may have a question regarding the issue and/or require a dev lead to help review the issue/ question you had.

An optional thing I would like to bring up is to add an explanation for why we are making the change. It can be brief and can explain why an issue was completed a particular way and/or why we are working on the issue (why fix the broken link?). Not every pull request has the why and it's not always required but I find it helpful when reviewing a pull request. In this case, it's pretty clear why we are doing this in the issue so I'm okay with it as-is.

@Adastros
Copy link
Member

This isn't related to the issue for the pull request but I noticed that some of the link values for the provider-link keys use single quotes or no quotes at all. For the title-link keys, all the link values are consistent and do not wrap the links in quotes. Wanted to bring this up as there are a few discussions regarding styling and consistency going around (such as in pull request #3839). There are some other things such as extra spaces or comments (see trophy.yml). Hopefully this will be covered in the upcoming style guide soon.

@steven-positive-tran
Copy link
Member Author

Thank you the task list has been checked and I have moved the issue to the In progress column

Copy link
Member

@Adastros Adastros left a comment

Choose a reason for hiding this comment

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

Perfect! Thanks for implementing the updates! Everything looks good to me

@roslynwythe roslynwythe self-requested a review January 26, 2023 04:45
Copy link
Member

@roslynwythe roslynwythe left a comment

Choose a reason for hiding this comment

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

Branches are correct, code change is perfect, and in the browser, the text "Error" is correctly linked to the new URL. Great job !

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

Labels

good first issue Good for newcomers P-Feature: Credit https://www.hackforla.org/credits/ role: back end/devOps Tasks for back-end developers role: front end Tasks for front end developers size: 1pt Can be done in 4-6 hours

Projects

Development

Successfully merging this pull request may close these issues.

Error Credit: Fix name link

4 participants