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

fix: broken link #218

Merged
merged 1 commit into from Jan 20, 2023
Merged

fix: broken link #218

merged 1 commit into from Jan 20, 2023

Conversation

vimode
Copy link
Contributor

@vimode vimode commented Jan 20, 2023

Fixes #217

@vercel
Copy link

vercel bot commented Jan 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
portfolio-ideas ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 20, 2023 at 2:50PM (UTC)

@github-actions
Copy link

Hello @vimode, 🔥 thanks for raising a pull request in this project. Now, sit back and drink some coffee while we review this.

Copy link
Owner

@Evavic44 Evavic44 left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

@Evavic44 Evavic44 merged commit e8e9e01 into Evavic44:main Jan 20, 2023
@vimode
Copy link
Contributor Author

vimode commented Jan 21, 2023

Thank you @Evavic44

The same issue exists in portfolio.md
I assumed updating README should also update the same at portfolio.md but it seems like that didn't happen.

Should I make a new PR to update portfolio.md?

@Evavic44
Copy link
Owner

Hey, @vimode Your changes didn't reflect because we're using an npm command to copy the content of the README.md file to Portfolio.md whenever a change is made.

But this will only work when you make the changes from within your editor and not the Github GUI.

Essentially, running:

npm run deploy

should make the updates, and then another PR can be raised. It's kinda tedious, I know. Which is why it isn't specified on the contributing guidelines.

Though I'm thinking it may be a good idea to do so for contributors who have the same question.

@vimode
Copy link
Contributor Author

vimode commented Jan 21, 2023

Understood. I will make a PR with the updated deployed version.

.... Which is why it isn't specified on the contributing guidelines.
Though I'm thinking it may be a good idea to do so for contributors who have the same question

The current Contributing guidelines encourages usage of the Github web interface to update the README and add the new portfolios which I assume is to encourage people to easily contribute, especially for beginners.

So adding the information about updating the deployment might require changing the contributing guidelines completely. As it would not make much sense for the contributor to use the Github web interface to make the changes as they still need to make changes locally.

An alternative solution could be to setup github actions to run npm run deploy when the changes are merged?

@Evavic44
Copy link
Owner

Evavic44 commented Jan 21, 2023

Understood. I will make a PR with the updated deployed version.

.... Which is why it isn't specified on the contributing guidelines.
Though I'm thinking it may be a good idea to do so for contributors who have the same question

The current Contributing guidelines encourages usage of the Github web interface to update the README and add the new portfolios which I assume is to encourage people to easily contribute, especially for beginners.

So adding the information about updating the deployment might require changing the contributing guidelines completely. As it would not make much sense for the contributor to use the Github web interface to make the changes as they still need to make changes locally.

An alternative solution could be to setup github actions to run npm run deploy when the changes are merged?

GitHub actions might be a viable option. If you can, raise an issue so we discuss this further. Thanks

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.

Broken link in README file
2 participants