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

Rename components to use CamelCase convention #336

Merged
merged 14 commits into from Nov 20, 2019

Conversation

lucaspontoexe
Copy link
Contributor

UpperCamelCase, actually.
(Closes #201)

Copy link
Contributor

@sarathms sarathms left a comment

Choose a reason for hiding this comment

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

I could only find time to go through half of the files changed. I have some feedback for now, I will get to the rest later, maybe after the requested changes are made.

  • test-info, withIntl, utils - These files do not necessarily export components. So these can remain the way they are. withIntl is a HoC definition and usually starts with lowercase.
  • AsnSelector, UrlChart - This is not a big deal, but it should be consistent. In some places it is ASNSelector in others you used AsnSelector. Since they are acronyms, I prefer ASNSelector and URLChart.
    box.js - This doesn't export a Box component, instead exports a couple of variants. So maybe rename it to boxes.js (yes, lowercase).
    flag - This hasn't been renamed yet, but you already import from ../Flag in one of the files. Same with header and footer.

I understand this is a lot of tedious work and it is much appreciated. Thank you ❤️

@sarathms
Copy link
Contributor

I am guessing renaming TestInfo back to test-info should avoid the merge conflict now showing up here after I merged a PR recently.

@lucaspontoexe
Copy link
Contributor Author

Well, there is a problem in my git configuration - don't merge this yet

@sarathms sarathms added this to In progress in OONI Explorer via automation Oct 17, 2019
@sarathms
Copy link
Contributor

When you think it is ready for review just use the "re-request" button(last step) to notify me.

@lucaspontoexe
Copy link
Contributor Author

I discovered the problem. I thought it was my Git configuration, but turns out that I was editing on a case-insensitive file system. Sorry about the mess - everything is properly renamed.

Copy link
Contributor

@sarathms sarathms left a comment

Choose a reason for hiding this comment

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

Thanks @lucaspontoexe for the PR. I added a few fixes because I felt some files need not be renamed, and a few imports needed to use new filenames. This is ready for merge.

@sarathms sarathms merged commit 136d6d3 into ooni:master Nov 20, 2019
@sarathms sarathms mentioned this pull request Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
OONI Explorer
  
In progress
Development

Successfully merging this pull request may close these issues.

Component filenames should be CamelCase
2 participants