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

gitcoin.co/profile/owocki => gitcoin.co/owocki #5278

Merged
merged 8 commits into from
Oct 22, 2019

Conversation

owocki
Copy link
Contributor

@owocki owocki commented Sep 27, 2019

makes gitcoin profiles a first class path on gitcoin.co; thereby giving users more sense of ownership of their gitcoin profile being a premium thing. and increasing the chances they pass it around like they do other social networks (twitter.com/owocki github.com/owocki facebook.com/owocki all have first class URL routes)

Refers/Fixes

self

Testing

tested locally; the only hard part is handling url collisions.. which i think dashboard.models.profile.url does quite elegantly

@owocki
Copy link
Contributor Author

owocki commented Oct 2, 2019

@gitcoinco/engineers feedback?

1 similar comment
@owocki
Copy link
Contributor Author

owocki commented Oct 2, 2019

@gitcoinco/engineers feedback?

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

resolved conflicts !
I'm not too keen on having multiple URL to access the same page but defer to the others on this

Doesn't that hurt SEO ? I remember dean telling me this
@octavioamu

@danlipert
Copy link
Contributor

@thelostone-mc @owocki good point! I think its okay if we add a canonical url tag

@owocki
Copy link
Contributor Author

owocki commented Oct 15, 2019

I'm not too keen on having multiple URL to access the same page but defer to the others on this

except for the one time migration; where will we have multiple URLs pointing to the same place? to my knowledge this is only linked via profile.url

@codecov
Copy link

codecov bot commented Oct 21, 2019

Codecov Report

Merging #5278 into master will increase coverage by 0.06%.
The diff coverage is 84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5278      +/-   ##
==========================================
+ Coverage   29.81%   29.88%   +0.06%     
==========================================
  Files         236      236              
  Lines       19884    19908      +24     
  Branches     2838     2844       +6     
==========================================
+ Hits         5929     5949      +20     
- Misses      13711    13713       +2     
- Partials      244      246       +2
Impacted Files Coverage Δ
app/app/context.py 48.14% <ø> (ø) ⬆️
app/app/urls.py 87.75% <0%> (-1.83%) ⬇️
app/dashboard/models.py 50.45% <60%> (-0.01%) ⬇️
app/dashboard/utils.py 43.65% <94.73%> (+2.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6721d88...d71007f. Read the comment docs.

@owocki
Copy link
Contributor Author

owocki commented Oct 21, 2019

@thelostone-mc @danlipert f36a2ed adds a canonical tag

@octavioamu
Copy link
Contributor

My only concern is about collisions. What happens if this guy want to use gitcoin? https://github.com/profile

@owocki
Copy link
Contributor Author

owocki commented Oct 21, 2019

@octavioamu that is handled here : https://github.com/gitcoinco/web/pull/5278/files#diff-0d2d86c824ef942c5a3a01c83ddfe758R3141-R3145

basically if a collision is detected it uses the old format ( /profile/USERNAME )

@octavioamu
Copy link
Contributor

I think could be amazing if we can create username.gitcoin.co but again not sure about collisions ex https://github.com/hackathons

@owocki
Copy link
Contributor Author

owocki commented Oct 21, 2019

I think could be amazing if we can create username.gitcoin.co but again not sure about collisions ex https://github.com/hackathons

yea this is something that the whiteblock team requested at #4832 too

@owocki
Copy link
Contributor Author

owocki commented Oct 22, 2019

yayyy

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

still in two minds but democracy :P

@thelostone-mc thelostone-mc merged commit a4a4527 into master Oct 22, 2019
@thelostone-mc thelostone-mc deleted the kevin/profile_links branch October 22, 2019 20:59
@thelostone-mc thelostone-mc restored the kevin/profile_links branch October 23, 2019 10:57
@thelostone-mc
Copy link
Member

I'm gonna have to revert this cause

Screenshot 2019-10-23 at 5 57 02 AM

  • /grants takes me to a profile which doesn't exist
  • it does confuse the user -> if we were to search for /explorer and mistyped it -> it would take him to a profile

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.

4 participants