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.

Sign up for GitHub

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

Jump to bottom

Prevent empty query parameter being set on dashboard #11561

Merged
merged 4 commits into from
May 24, 2020
Merged

Prevent empty query parameter being set on dashboard #11561

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f18906
prevent empty query parameter being set on dashboard
zeripath May 22, 2020
12358f0
Merge branch 'master' into only-show-query-on-dashboard-if-query-empty
jolheiser May 24, 2020
0db49e6
Merge branch 'master' into only-show-query-on-dashboard-if-query-empty
zeripath May 24, 2020
2ae4b41
Merge branch 'master' into only-show-query-on-dashboard-if-query-empty
zeripath May 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion web_src/js/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2850,7 +2850,12 @@ function initVueComponents() {
params.set('repo-search-page', `${this.page}`);
}

window.history.replaceState({}, '', `?${params.toString()}`);
const queryString = params.toString();
if (queryString) {
window.history.replaceState({}, '', `?${queryString}`);
} else {
window.history.replaceState({}, '', window.location.pathname);
Copy link
Member

@silverwind silverwind May 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is this branch even necessary? This would kill any #hash present on the URL (thought the branch above would also)

Copy link
Member

Choose a reason for hiding this comment

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

Here's a helper function you can put in utils.js that preserves pathname, search and hash:

function setUrl(opts) {
  const url = new URL(window.location.href);
  if ('pathname' in opts) url.pathname = opts.pathname || '/';
  if ('search' in opts) url.search = opts.search || '';
  if ('hash' in opts) url.hash = opts.hash || '';
  const newLocation = String(url).replace(url.origin, '');
  window.history[opts.push ? 'pushState' : 'replaceState'](null, null, newLocation);
}

You can then call it like this to set/unset only the search part:

setUrl({search: params.toString() || ''});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hash is will be out of date and irrelevant if you are pressing these buttons - if anything we should be setting the hash to reference the repo-list table.

Imagine we finally get round to sticking ids on the newsfeed items and you have been sent the newsfeed via an anchor then have scrolled back up to the repo-list - keeping the anchor would mean that on going back you will scroll back to the anchor even though the actual proper context for those query elements is the repo-list.

As the repo-list is the top of the page it's not necessarily needed to set the anchor hash - but - keeping it could be confusing.

Copy link
Member

@silverwind silverwind May 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You can remove the hash too with it. This should be equal to your version:

setUrl({search: params.toString() || '', hash: ''});

Your first case would set a wrong path if gitea is running on a subdirectory as it would set path to parent root directory. I think my function is safer because it only sets the parts that you want to change, not all at once.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, it seems a replaceState with ?something as URL does not kill the path, at least in Firefox. But I would probably not want rely that this behaviour is the same in all browsers.

Copy link
Member

@silverwind silverwind May 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So I think your way is actually fine. I did not realize browsers were smart enough to do the right thing for ?search and #hash pseudo urls, but it appears to be working fine because the url standard requires to apply the base url so fragments like this will work.

> new URL('?search', "https://www.example.com/dir/?search").href
'https://www.example.com/dir/?search'
> new URL((new URL("https://www.example.com/dir/")).pathname, "https://www.example.com/dir/?search").href
'https://www.example.com/dir/'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hehe! one of the things that I've recently realised is that it is helpful to run your testing instance through a proxy mounted on a suburl (https://localhost/gitea) as it catches a lot of simple problems.

}
},

toggleArchivedFilter() {
Expand Down