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

Scroll to Top of Topic instead of Reloading #2050

Merged
merged 1 commit into from
Mar 5, 2014

Conversation

verg
Copy link
Contributor

@verg verg commented Mar 3, 2014

This PR changes the following links to use URLs with the first post rather than the topic url:

  1. Clicking a Topic's header.
  2. Quotes and Replies to the first post.
  3. Jump to Top button in the Progress Bar.

For example. We'll now link to /t/discourse-general-polish/13184/1/ instead of than /t/discourse-general-polish/13184. This loads the topic much more quickly than the full page reload.

From: https://meta.discourse.org/t/discourse-general-polish/13184

We unconditionally reload topic page if you click on "title". instead it should scroll you to top if post 0 is already in memory. Same for in between posts navigation (eg quoting and so on)

@discoursebot
Copy link

You've signed the CLA, verg. Thank you! This pull request is ready for review.

@coding-horror
Copy link
Contributor

This means copying the link from the topic header is now deep linking to the first post -- that's not desirable.

Cases 2 and 3 in your list are probably OK though since they aren't such obvious click-copy targets.


jumpToTopPost: function () {
var topic = this.get('topic');
if (topic) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not call the jumpTop method in the file below this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it normal to call functions on other controllers? That feels odd to me.

@verg
Copy link
Contributor Author

verg commented Mar 3, 2014

@coding-horror Do you think something like this would work?

<a class='topic-link' href='{{unbound topic.url}}' {{action jumpToTopPost}}>{{{topic.fancy_title}}}</a>

Enter the Topic url in the href, but link to an action that loads the first post. I just tested it out in my dev and it works functionally.

@coding-horror
Copy link
Contributor

provided right-click copy of the URL in the topic header returns the root URL, with no /1 on it, that should be OK. We usually test in Safari, IE10+, FF, Chrome.

@SamSaffron
Copy link
Member

@verg yes that is far preferable we can not give up right click, also ... please test cases where posts are not loaded (eg: very long topic)

@verg
Copy link
Contributor Author

verg commented Mar 4, 2014

Great. Thanks! I updated the PR so the title links have the topic-level href. Looks good on Safari/IE/FF/Chrome and on long topics.

@SamSaffron
Copy link
Member

OK, trying this out! thanks!

SamSaffron added a commit that referenced this pull request Mar 5, 2014
Scroll to Top of Topic instead of Reloading
@SamSaffron SamSaffron merged commit 24356cd into discourse:master Mar 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants