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

Add link-to-github-io feature #3586

Merged
merged 5 commits into from
Sep 29, 2020

Conversation

yashshah1
Copy link
Contributor

Thanks for contributing! 🍄

  1. LINKED ISSUES:
    Closes Quick hyperlink to visit someone's github.io #3560

  2. TEST URLS:
    Repository Lists
    Repository Page

  3. SCREENSHOT:
    image
    image

@yakov116
Copy link
Member

yakov116 commented Sep 21, 2020

@yashshah1 Thanks for the PR. 🎉

There are many list issues, try to disable pettier for this project.

You can fix all list issues with xo --fix
You may need to install xo globally. npm i -g xo
Or you can try npm run lint.

source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/refined-github.ts Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
@fregante
Copy link
Member

You can fix all list issues with xo --fix
You may need to install xo globally. npm i -g xo
Or you can try npm run lint.

You can also just use npx xo --fix in the current directory instead of installing it globally.

source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
Copy link
Member

@yakov116 yakov116 left a comment

Choose a reason for hiding this comment

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

Can you run npx xo --fix?

source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
@yashshah1
Copy link
Contributor Author

@yakov116 I did before pushing the changes, but that didn't change the indentation of pageDetect for some reason. Pushed manually right now

@yakov116
Copy link
Member

  source/features/link-to-gihub-io.tsx:17:2
  ✖  17:2  Expected space(s) after "for".  @typescript-eslint/keyword-spacing
  ✖  80:2  Missing semicolon.              @typescript-eslint/semi

Both theses should be fixed with xo --fix

@yashshah1
Copy link
Contributor Author

@yakov116
Sorry for so many of the minor issues, running npx xo --fix or npx xo didn't show any of the errors that come up on the checks at GH. Running npm run lint however shows me the same list of errors, and currently only the ones flagged by indentaion come up (in features.add).

Thank you so much for your patience and reviews.

Copy link
Member

@yakov116 yakov116 left a comment

Choose a reason for hiding this comment

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

We can combined all of these

source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
@yashshah1
Copy link
Contributor Author

Done!

@fregante fregante changed the title Add quick-links-to-users-github-io feature Add link-to-github-io feature Sep 22, 2020
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
@yakov116
Copy link
Member

@yashshah1 looking great 🎉

You are still missing a screen shot.
You can look at other features how the screenshot is done. It should be zoomed in so we can see content and the new feature that was added should be noted.

@yashshah1
Copy link
Contributor Author

Phew, finally.
I had a question, how do I include multiple screenshots (given that this affects the UI in two places), do I simply make a collage out of it?

Also, I haven't yet added this into the README yet, will add it into the Repository section

@yakov116
Copy link
Member

yakov116 commented Sep 22, 2020

do I simply make a collage out of it?

If you can do it clearly yes.

Also, I haven't yet added this into the README yet, will add it into the Repository section

Not sure @fregante WDYT? Profiles or Repository?

@fregante
Copy link
Member

Not sure @fregante WDYT? Profiles or Repository?

Repository makes more sense because it affects each repository after all, even if it's a list.

do I simply make a collage out of it?

You can do a simple horizontal split-screen with these two:

Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

The space was in the wrong place

source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
@fregante
Copy link
Member

Thought: what if we unconditionally add the icon next to the repo name?

Screen Shot 2020-09-22 at 14 51 14

So the style is consistent and the link is accessible from any page of the repo.

@yashshah1
Copy link
Contributor Author

@fregante That does sound like a better idea! I'm assuming this is as a replacement to adding it onto the sidebar.

@yashshah1
Copy link
Contributor Author

Considering that the UI is fixed, I've added a screenshot and an entry in the README.

@yakov116
Copy link
Member

@yashshah1 dont force push it makes it very had to review what changed

readme.md Outdated Show resolved Hide resolved
source/features/link-to-gihub-io.tsx Outdated Show resolved Hide resolved
* changed function names
* added observeElement
@yashshah1
Copy link
Contributor Author

Sorry for the delay, but I've incorporated the changes you've suggested. Hoping this does it!

@yakov116 yakov116 self-assigned this Sep 29, 2020
@yakov116 yakov116 removed their assignment Sep 29, 2020
Copy link
Member

@yakov116 yakov116 left a comment

Choose a reason for hiding this comment

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

@yashshah1 the last change I made was to use Select-observer over observeElement. This was something that's change on the master branch.

🎉

@yakov116 yakov116 merged commit 4281ae1 into refined-github:master Sep 29, 2020
@yakov116
Copy link
Member

@yashshah1 congrats on adding your first refined-github feature!

@yashshah1
Copy link
Contributor Author

@yakov116 Thank you! This has been quite a source of learning for me, I'm looking forward to contributing more in the days to come :D

@yashshah1 yashshah1 deleted the 3560-hyperlink-to-github-io branch September 29, 2020 19:13
kidonng pushed a commit to kidonng/refined-github that referenced this pull request Oct 7, 2020
Co-authored-by: Yakov <16872793+yakov116@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Quick hyperlink to visit someone's github.io
4 participants