Skip to content
This repository has been archived by the owner on Jul 25, 2022. It is now read-only.

Remove Page::is_active() #1716

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove Page::is_active() #1716

wants to merge 1 commit into from

Conversation

ip1981
Copy link
Member

@ip1981 ip1981 commented Mar 24, 2020

The purpose of this method is to prevent reloading of the current page if
the same URL is clicked. But this has downsides and, probably, no value:

  • It is easy to forget to update this method.
  • Needs access to models' fields which better be private.
  • An opened URL is still clickable but confusingly has no effect.
  • There is no way to reinitialize a page other than total reload (or go to another page first).

Signed-off-by: Igor Pashev [email protected]


This change is Reviewable

@ip1981 ip1981 added tech debt triage Issues to be discussed further labels Mar 24, 2020
@ip1981 ip1981 requested review from jgrund and a team March 24, 2020 08:14
@ip1981 ip1981 self-assigned this Mar 24, 2020
@ip1981 ip1981 force-pushed the ip1981/active-page branch from 8005720 to 6f6d250 Compare March 24, 2020 08:17
nlinker
nlinker previously approved these changes Mar 24, 2020
Copy link
Contributor

@nlinker nlinker left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jgrund)

mkpankov
mkpankov previously approved these changes Mar 26, 2020
Copy link
Contributor

@mkpankov mkpankov left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jgrund)

@ip1981 ip1981 dismissed stale reviews from mkpankov and nlinker via fa1970d April 27, 2020 15:38
@ip1981 ip1981 force-pushed the ip1981/active-page branch from 6f6d250 to fa1970d Compare April 27, 2020 15:38
mkpankov
mkpankov previously approved these changes Apr 28, 2020
Copy link
Contributor

@mkpankov mkpankov left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jgrund)

@jgrund jgrund removed the tech debt label Jun 25, 2020
@ip1981 ip1981 marked this pull request as draft July 1, 2020 17:54
The purpose of this method is to prevent reloading of the current page if
the same URL is clicked. But this has downsides and, probably, no value:

* It is easy to forget to update this method.

* Needs access to models' fields which better be private.

* An opened URL is still clickable but confusingly has no effect.

* There is no way to reinitialize a page other than total reload (or go
  to another page first).

Signed-off-by: Igor Pashev <[email protected]>
@ip1981 ip1981 force-pushed the ip1981/active-page branch from fa1970d to 736dc30 Compare July 6, 2020 14:45
@ip1981 ip1981 marked this pull request as ready for review July 6, 2020 14:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
triage Issues to be discussed further
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants