refactor: replace capital letters with normal letters on our navbar #296

ftonato · 2021-10-12T13:32:38Z

Description

In order to fix the problem related on the issue #295 I've created the code to fix it.

Fixes #295

Type of Change:

Code

How Has This Been Tested?

You can run the new branch and check if all navigation items are with "capital letters".

Checklist:

Delete irrelevant options.

My PR follows the style guidelines of this project
I have performed a self-review of my own code or materials

src/Components/Header/index.js

isabelcosta

Thank you so much @ftonato for proposing and working on this!

isabelcosta · 2021-10-15T00:23:46Z

cc @brittanyjoiner15 could you maybe look at this one too :)

src/Components/Header/index.js

brittanyjoiner15

Looks good to me! Left a small comment about alphabetization, but feel free to ignore as it's not essential :) Well done, this was nice @ftonato !

isabelcosta · 2021-10-17T10:43:05Z

@brittanyjoiner15 could you please approve it again :) Approve is one of the review options. After that I can merge :)

isabelcosta · 2021-10-17T13:57:07Z

@brittanyjoiner15 do you know what could be wrong with prettier run here? I don't think it is related to this PR 🤔 But I am not sure
cc @Aaishpra @codesankalp in case you have some useful knowledge here on github actions in web projects

@ftonato do you have an idea of what could be happening here? I updated this PR in hopes it would be mergable right away, but this check is failing. Could we do something here 🤔

brittanyjoiner15 · 2021-10-17T15:53:03Z

@ftonato @isabelcosta hmm, I wonder if it's because it's confused because we have the .prettierrc.json file merged in but this branch doesn't have it yet? Maybe @ftonato try pulling changes fromdevelop and seeing if it works after that's all merged up together?

ftonato · 2021-10-17T16:03:48Z

Hello, after a few minutes of trying and trying I found the problem, or at least managed to fix it on my branch.

I'll open a pull-request to submit the solution and remove the commits I made on the current branch to test...

btw, we should have a telegram channel to talk about a few things (all three)!

/c @isabelcosta @brittanyjoiner15

ftonato · 2021-10-17T16:11:02Z

I just deleted all commits, I'm going to redo them (dumb)!

Edit: Done! We can merge this and as I said above I will send another PR to fix the "prettier problem" 😛

isabelcosta · 2021-10-17T20:04:41Z

btw, we should have a telegram channel to talk about a few things (all three)

We can talk on Zulip :) That is where the community talks about the projects so that everyone can join discussions. We have a stream for this project.

isabelcosta · 2021-10-18T22:34:58Z

@ftonato can you update the PR, just so we see the checks passing, please?

isabelcosta · 2021-10-19T20:31:56Z

@ftonato do you have any idea why the checks for prettier are failing 🤔 i wonder why 😳

brittanyjoiner15 · 2021-10-19T20:37:11Z

@isabelcosta so we probably need to update the docs to let people know they'll want to install prettier locally, and they might need to run the npx --write (can't remember the exact command, but something like that). It seems like all of them are failing now that we've implemented prettier aren't they? Or have we had any successful ones aside from the ones from my device?

ftonato · 2021-10-19T20:39:17Z

I don't understand why it's failing only on the pull-request and not on my branch (fork), I'll need to invest a little more time on this tomorrow to calmly evaluate...

I will do this as soon as possible!

isabelcosta · 2021-10-19T20:46:37Z

@isabelcosta so we probably need to update the docs to let people know they'll want to install prettier locally, and they might need to run the npx --write (can't remember the exact command, but something like that). It seems like all of them are failing now that we've implemented prettier aren't they? Or have we had any successful ones aside from the ones from my device?

If that is something we can put on README, let's do it! Could you create an issue to add that instruction to the README?

isabelcosta · 2021-10-19T20:48:27Z

I don't understand why it's failing only on the pull-request and not on my branch (fork), I'll need to invest a little more time on this tomorrow to calmly evaluate...

I will do this as soon as possible!

I see it falling on your fork as well https://github.com/ftonato/anitab-org.github.io/runs/3919521463 🤔 but take your time! As long as we have updates from time to time.

brittanyjoiner15 · 2021-10-19T20:49:26Z

@isabelcosta yes! i can do that, @ftonato i might want you to be my "beta" tester and confirm if that seems to fix the issue or not before i do that. Basically following these instructions

ftonato · 2021-10-20T08:02:22Z

I made some changes thinking it might have been something and it worked on my branch, but here it kept failing so I pushed it back to the original state (thus failing on my branch too).

However, I will try to validate all this today =)

ftonato · 2021-10-20T08:11:46Z

@isabelcosta can you take a look on this? It seems that our bot (prettier action) don't have the right permissions to perform the commit:

isabelcosta · 2021-10-22T09:04:04Z

Not sure why, but it seems the src/content/home.js needs prettier updates 🤔 Could you try running prettier on your branch, including src/content/home.js and see if it changes anything? (i know you did not change that file at all, but if it helps, I don't mind getting that into this PR) @ftonato

From what I understand, this is failing because it is trying to commit changes for a file that has changes proposed by prettier.

I wonder if this action should be changed, to not try to commit but simply fail if prettier suggests changes and pass if prettier does not suggest changes.

isabelcosta · 2021-10-22T09:26:09Z

@ftonato I see it failing again. I will ignore this for now and test the PR and if all good merge this.
Will create a follow-up issue to fix the action 😅 I see it's failing in multiple PRs :(

isabelcosta · 2021-10-22T09:29:46Z

.github/workflows/deploy.yml

        uses: creyD/[email protected]
        with:
-          prettier_options: "--write src/**/*.{ts,jsx,js,css,html,json,md}"
+          prettier_options: --write src/**/*.{ts,jsx,js,css,html,json,md}


I just noticed, is there any reason why this was reverted and why package-lock.json and yarn.lock have so many changes?

I realized that the quotes are not necessary (and not suggested in the plugin's official documentation) so I removed it!

The package-lock has a lot of changes because I tried to rebase it with develop (it still fails)

Could you revert those file changes (yarn.lock and package-lock.json)? And keep this PR to the changes related to the ticket?

ftonato · 2021-10-22T09:36:58Z

@isabelcosta I really don't know why it only fails in PR for this repository, I do it in my FORK it works perfectly, and I really think it might have some relationship with permissions (https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#permissions).

My suggestion is to stop using github action to make the prettier and do it with husky, does it sound okay or not? (If approved, I can create the PR with it, but someone from the organization will have to remove the action)

isabelcosta · 2021-10-22T09:42:07Z

@isabelcosta I really don't know why it only fails in PR for this repository, I do it in my FORK it works perfectly, and I really think it might have some relationship with permissions (https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#permissions).

My suggestion is to stop using github action to make the prettier and do it with husky, does it sound okay or not? (If approved, I can create the PR with it, but someone from the organization will have to remove the action)

I understand, we can remove/fix it on another ticket. That is fine!
Once I have a little more time I'll look into permissions and GH action.

ftonato · 2021-10-22T09:43:28Z

I will create another PR to include all this changes! (#325)

@isabelcosta Can we merge this PR?

…l-letters

isabelcosta reviewed Oct 13, 2021

View reviewed changes

src/Components/Header/index.js Show resolved Hide resolved

isabelcosta reviewed Oct 13, 2021

View reviewed changes

src/Components/Header/index.js Show resolved Hide resolved

isabelcosta approved these changes Oct 13, 2021

View reviewed changes

isabelcosta requested a review from vj-codes October 13, 2021 23:24

isabelcosta added the Status: Needs Review PR needs an additional review or a maintainer's review. label Oct 13, 2021

isabelcosta requested review from Aaishpra and Amulya-coder October 14, 2021 23:01

brittanyjoiner15 reviewed Oct 15, 2021

View reviewed changes

src/Components/Header/index.js Outdated Show resolved Hide resolved

brittanyjoiner15 reviewed Oct 15, 2021

View reviewed changes

isabelcosta added hacktoberfest-accepted Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Oct 17, 2021

brittanyjoiner15 approved these changes Oct 17, 2021

View reviewed changes

ftonato force-pushed the refactor/update-navibar-capital-letters branch from 30b934e to 78f46c4 Compare October 17, 2021 16:08

ftonato force-pushed the refactor/update-navibar-capital-letters branch from 78f46c4 to 7d7faa6 Compare October 17, 2021 16:14

ftonato mentioned this pull request Oct 17, 2021

chore: update action to ensure we get the reference before running "prettier" #305

Merged

ftonato mentioned this pull request Oct 17, 2021

GitHub Actions for Prettier process is failing / not respecting some rules that should #310

Closed

ftonato force-pushed the refactor/update-navibar-capital-letters branch 2 times, most recently from 9a9e8fe to 7d7faa6 Compare October 19, 2021 08:28

ftonato added 4 commits October 20, 2021 09:06

refactor: replace capital letters with normal letters on our navbar

e21210d

chore: update package-lock & yarn.lock

200e993

chore: update github wrokflow files

49fa1fd

chore: prettify our files

217b463

ftonato force-pushed the refactor/update-navibar-capital-letters branch from 7d7faa6 to 217b463 Compare October 20, 2021 08:09

chore: update home file with prettier

5c46448

isabelcosta added Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Oct 22, 2021

isabelcosta reviewed Oct 22, 2021

View reviewed changes

Merge branch 'anitab-org:develop' into refactor/update-navibar-capita…

319b46c

…l-letters

ftonato mentioned this pull request Oct 22, 2021

Replace Prettier Action with pretty-quick pre-commit #326

Closed

vj-codes closed this Sep 30, 2022

refactor: replace capital letters with normal letters on our navbar #296

refactor: replace capital letters with normal letters on our navbar #296

Uh oh!

Conversation

ftonato commented Oct 12, 2021

Description

Type of Change:

How Has This Been Tested?

Checklist:

Uh oh!

Uh oh!

Uh oh!

isabelcosta left a comment

Choose a reason for hiding this comment

Uh oh!

isabelcosta commented Oct 15, 2021

Uh oh!

Uh oh!

brittanyjoiner15 left a comment

Choose a reason for hiding this comment

Uh oh!

isabelcosta commented Oct 17, 2021

Uh oh!

isabelcosta commented Oct 17, 2021

Uh oh!

brittanyjoiner15 commented Oct 17, 2021

Uh oh!

ftonato commented Oct 17, 2021

Uh oh!

ftonato commented Oct 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isabelcosta commented Oct 17, 2021

Uh oh!

isabelcosta commented Oct 18, 2021

Uh oh!

isabelcosta commented Oct 19, 2021

Uh oh!

brittanyjoiner15 commented Oct 19, 2021

Uh oh!

ftonato commented Oct 19, 2021

Uh oh!

isabelcosta commented Oct 19, 2021

Uh oh!

isabelcosta commented Oct 19, 2021

Uh oh!

brittanyjoiner15 commented Oct 19, 2021

Uh oh!

ftonato commented Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ftonato commented Oct 20, 2021

Uh oh!

isabelcosta commented Oct 22, 2021

Uh oh!

isabelcosta commented Oct 22, 2021

Uh oh!

isabelcosta Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

ftonato Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

ftonato Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

isabelcosta Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

ftonato commented Oct 22, 2021

Uh oh!

isabelcosta commented Oct 22, 2021

Uh oh!

ftonato commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

ftonato commented Oct 17, 2021 •

edited

Loading

ftonato commented Oct 20, 2021 •

edited

Loading

ftonato commented Oct 22, 2021 •

edited

Loading