[openstreetmap/openstreetmap-website] Re-arrange login and signup screens as discussed in #4128 (PR #4455)

Milan Cvetkovic notifications at github.com
Fri Mar 15 14:29:45 UTC 2024


> > @tomhughes @gravitystorm any more changes required for this PR?
> 
> If you can find any way to move parts of this PR into separate PRs, then that would be great. For example, you fixed the blocked page (headings and paragraphs) in the latest commit, but that could be a completely separate PR that can be reviewed separately and merged first. Are there other things like that here?
> 
> Perhaps the "login" vs "log in" text changes? The logo change? Or other similar things that can be treated separately? Large pull requests are quadratically hard to deal with - not only is there more to review, but each time I come to re-review the PR, more things have changed, and I need to read and re-read both the changed and also the unchanged parts over and over. So it's really worthwhile to break down the PR into smaller parts, that will ideally only need one maintainer to review and merge.

I have created 3 separate PRs as you suggested:
- #4455
- #4578
- #4580

I understand the frustration with large PRs like this. They are not pleasant neither for reviewers nor the contributor.

The moving target for contributor (master branch being updated resulting in merge conflicts) triggers me to rebase the PR branch, breaking the correlation between comments and code, and that makes reviewing the PR even more unpleasant, usually resulting in full re-review.

In case of this PR there is a really big modification that cannot be reasonably split into smaller PRs, namely "joining sign-up and terms screens into one" which touches large portions of `users_controller.rb` and related views.

However, it seems that the small PRs I created above will not reduce the size of this PR significantly, since this PR would modify or move the same pieces of code. I will have to wait until they are merged before rebasing this PR once again...



-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/4455#issuecomment-1999788970
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/4455/c1999788970 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20240315/1c717eac/attachment.htm>


More information about the rails-dev mailing list