Skip to content

Conversation

@ashwamegh
Copy link

@ashwamegh ashwamegh commented Jul 14, 2017

@ashwamegh ashwamegh changed the title Added animation to section scrolls [Fix: #45] Added animation to section scrolls Jul 14, 2017
@realslimshanky
Copy link
Member

realslimshanky commented Jul 14, 2017

@shashank7200 here is the problem with your implementation of smooth scroll.

  1. It only works on the index.html i.e. the homepage of in.pycon.org
  2. For every other page the link for speakers is dead i.e. clicking on 'speakers' does nothing/ takes you nowhere.
    You can try to make it work by applying smooth scroll only on the index.html.

@ashwamegh ashwamegh changed the title [Fix: #45] Added animation to section scrolls [Fix: #45] Added animation to section scrolls [Smooth Scrolling] Jul 15, 2017
Copy link
Member

@realslimshanky realslimshanky left a comment

Choose a reason for hiding this comment

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

Trying to make smooth scroll work like this is leaving dead links behind. A different approach is needed to make this work.

js/main.js Outdated
ADD ANIMATIONS TO SECTION SCROLL
-------------------------------------- */
$('a[href*=\\#]').on('click', function(event){
event.preventDefault();
Copy link
Member

Choose a reason for hiding this comment

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

This makes 'Speakers' button a dead link when clicked from any other page than index.html. Different approach is needed to make it work.

Copy link
Author

Choose a reason for hiding this comment

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

@realslimshanky okay, let me fix that.

@realslimshanky
Copy link
Member

@shashank7200 according to your new changes, on clicking on 'Speakers' takes us to the Speaker tab on index.html page but smooth scroll is not working. i.e. the feature that you are proposing works only when we are at index.html and not on any other page (which is 6 out of 7 case).

@ashwamegh
Copy link
Author

@realslimshanky solved it!

@realslimshanky
Copy link
Member

realslimshanky commented Jul 15, 2017

There are few glitches on the mobile browsers while testing. I would wait to see other people's review.

/* -------------------------------------
ADD ANIMATIONS TO SECTION SCROLL
-------------------------------------- */
function ea_scroll( hash ) {
Copy link
Member

Choose a reason for hiding this comment

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

Please indent your JS correctly :)

@CuriousLearner
Copy link
Member

@shashank7200 one level backward. Should go at the same level as your comment.

@ashwamegh
Copy link
Author

@CuriousLearner When I indented the code in the editor, it was perfect but, after pushing code to github, it is showing unindented.

@ashwamegh ashwamegh force-pushed the master branch 5 times, most recently from 24c1a59 to 9ca8ab0 Compare July 18, 2017 06:23
@ashwamegh
Copy link
Author

@CuriousLearner done!

@CuriousLearner
Copy link
Member

Hey,

Thanks for your contributions!

As I see on the link you mentioned, whenever I click on the #speakers link as you mentioned on the issue, it redirects me to your portfolio site.

Can you please have a look at this?

@ashwamegh
Copy link
Author

@CuriousLearner See that is not a problem, that is a behaviour of that website which was earlier implemented, I can change it but, it'll be not good to change it. You can test it locally, it'll work fine. I had long discussion with @realslimshanky explaining this behaviour. From a talk to @realslimshanky , what I explained to him:

  1. See, you can expect speakers section to be available on my github https://shashank7200.github.io/
  2. The speakers link is implemented as href=/#speakers, so it will always point to your home directly and then search for speakers section
  3. Github creates a route for every repo other than your own github page i.e. in my case https://shashank7200.github.io/ {i.e. home route]
    and my fork repo of python website is is at https://shashank7200.github.io/inpycon2017 which is not acting as the home directory
  4. That's why either you can review it locally or merge the pull request, it'll automatically be available on the website.

@CuriousLearner
Copy link
Member

I know this would show up correctly once deployed or on local testing. My main concern here is, when we archive this site, it should work correctly that time too, which not might be the case.

@ashwamegh
Copy link
Author

@CuriousLearner In this case, what should I do then?

@CuriousLearner
Copy link
Member

@shashank7200 Rather than referencing it from the root, add an anchor with respect to the current page.

@ashwamegh
Copy link
Author

@CuriousLearner I am doing what you suggested, but I must say, it will not redirect you to the homepage's speakers section afterwards from external links

@CuriousLearner
Copy link
Member

Yes, you're right. Let me think how can we do this.

@ashwamegh
Copy link
Author

Okay @CuriousLearner

@realslimshanky
Copy link
Member

Sorry for delay @shashank7200
After reviewing current change here are my concerns.

  • You need to change the href property on a tag of speakers from #speakers to /2017/#speakers. In this case even after archiving the links will work.
  • Then I'm afraid that we'll be back to square 1 when the smooth scroll wasn't working 6 out of 7 times.
    Try this fix and let me know.

@ashwamegh
Copy link
Author

ashwamegh commented Aug 4, 2017

@realslimshanky Done changes locally, but it breaks the functionality. I can suggest we can do the needful changes while archiving and leave the href as /#speakers or #speakers for now. Should I push these changes to remote?

Here's the output of locally tested changes:

Local inpycon test

@realslimshanky
Copy link
Member

realslimshanky commented Aug 4, 2017

This is temporary but might work.
What do you think @CuriousLearner ?
Should we go for temporary fix and change the links before archiving next year
or the permanent one and test it after merging?

@CuriousLearner
Copy link
Member

I'm not sure.

@theskumar How should we go around this/

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.

3 participants