[bisq-network/bisq-website] New multilanguage system (#260)
notifications at github.com
Wed Oct 9 06:02:10 UTC 2019
> What is the problem we are trying to solve?
While reviewing this PR, we realized there might be flaws in the PR review process since so many issues were missed.
For starters, judging from the amount of changes requested so far that seem to be obvious bugs, it seems this PR wasn't tested before being submitted for review, or each time before re-review. Also, after reviewers pointed out the seemingly unintentional changes introduced by this PR, it's not clear to everybody why unintentionally changing things is bad, so now I think the problem at hand is to reach consensus on how exactly to review this PR and review/approve these changes one by one.
To solve this, I'm proposing for us to use a very pragmatic before/after diff of the website output, in addition to visually reviewing the PR output, and review those changes one by one, so we can see exactly what might be changing "under the hood" in addition to the obvious visual changes. In addition, I think it would be a good idea if PR submitters do this themselves before submitting the PR, so they can identify and fix unintentional changes before the review process.
> Could you point out a problem or a difference in the final output caused by those "issues"?
Sure, but I think I've already pointed out enough of your issues for you, so for now I'm asking you to *test it yourself* and check the diff output before we re-review. Ideally a refactoring should be exactly the same in terms of functionality and output changes, the goal should be to have a null diff. If there are exceptions to that they need to be reviewed and approved one by one.
But if it wasn't clear enough, I'll take a screenshot of my previous comment and highlight all the changes in red and try to clarify the differences in this one example as much as possible:
<img width="463" alt="Screen Shot 2019-10-09 at 14 53 09" src="https://user-images.githubusercontent.com/232186/66455146-e0e2b200-eaa4-11e9-8e47-0d699ec23054.png">
1) The `href="/dao/"` has changed to `href="/"`
This is a different URL, which means the user will go to a different page. If you already fixed this one, great :+1:
2) The `hreflang="en"` has been removed
I added this tag to improve SEO a couple of months ago, and it fixed the issues we were having on Google. It's not cool if you just randomly remove it as part of this PR. https://github.com/bisq-network/bisq-website/pull/215/files#diff-8a279082e95baa3b49f6a99e29febe18R16
3) The indentation has been changed, and additional newlines have been added
While indentation and newlines may not seem important to you, they are very important when checking what has actually changed. If you arbitrarily change tabs into spaces, this is very bad for version control and reviewing changes.
I could go on, but since there are 10+ of these diff snippets that need to be reviewed individually, and since the reviewers of this PR have already spent considerable time, I'm now asking you to go back and test and improve the PR further on your own using the diff output process, in order to reduce the amount of unintentional changes, before we re-review it again. I think this would have saved the reviewers a lot of time pointing out the obvious UI issues to you - this thread is 79 comments so far, so I think we've spent enough time reviewing for now.
> Also, what do you think about fixing minor or irrelevant differences, like the different hreflang, in a subsequent PR, or are these issues important enough to keep this one on hold?
Well, that's exactly my point, isn't it? All of "these issues" need to be reviewed, one by one, and only after we identify and discuss each of them, can we decide if they are important or not. Currently, since nobody even knew about them until I pointed this out, it means we weren't properly reviewing the PR and need to change our review process.
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the bisq-github