jonorthwash / ud-annotatrix

GNU General Public License v3.0
61 stars 49 forks source link

Static mode, server, building, UI/UX #356

Closed yaskevich closed 5 years ago

yaskevich commented 5 years ago
yaskevich commented 5 years ago

Currently, it's a new set of changes, I didn't take into account that previous set was not merged yet. So maybe it would be better to revert last commit, to wait for previous set with its PR would be accepted and then re-commit?

jonorthwash commented 5 years ago

Really you should have committed the additional stuff as separate PR. But at this point I don't think it makes sense to revert the commit. Could you fix the indentation and spelling issues (and anything else you said you would), and then I'll merge the PR.

Also, it looks like your branch is missing a couple commits from this one?

jonorthwash commented 5 years ago

@yaskevich, could you revert the reformatting of index.html? And as discussed in our meeting the other day, please stop adding to this PR—open new PRs for additional features and fixes.

maryszmary commented 5 years ago

@jonorthwash, what should be finished for pr to be merged?

yaskevich commented 5 years ago

@yaskevich, could you revert the reformatting of index.html?

Unfortunately, I added some changes in subsequent commits to index.ejs, so now it is too hard to reconstruct original indentation.

jonorthwash commented 5 years ago

Unfortunately, I added some changes in subsequent commits to index.ejs

This is exactly why it's a bad idea to make formatting changes and substantive changes in the same commit. Please revert the commit and figure out what you did. Also, I asked about index.html, not index.ejs...

jonorthwash commented 5 years ago

Can't you just mass-replace spaces with tabs?

yaskevich commented 5 years ago

The reason why formating changed: https://github.com/jonorthwash/ud-annotatrix/pull/356#discussion_r295577729

Why I mentioned ejs https://github.com/jonorthwash/ud-annotatrix/pull/356#discussion_r298281610

Can't you just mass-replace spaces with tabs?

At the beginning of the work on the project I was wondering whether the formatting is consistent in the whole project. After I got familiar with a whole codebase I could claim that it does not (sometimes there are tabs, sometimes there are spaces). Unification would drastically change all the code base (in diffs).

If change of the formatting caused a problem for making diff and therefore for code review, (as a solution) I could pretty-print any file and its earlier version with the same prettifier.

I understand that it is great when every file of the project follows the same formatting rules. Unfortunately, JavaScript (the more so HTML) is not Python, and if someone prefers one style, and someone does another, the browser/interpreter doesn't throw any errors, so it leads to some non-conformity. Also, maybe, sometimes I change formatting because in such way it is more clear for me and it helps with adding new code.

Actually, I have noticed some inconsistencies and withdrawal of common practices in the code base that no one has mentioned yet. But considering that the project has quite specific tasks to deal with (interaction with Github API or debugging tricky issues which rooted in notatrix), it seems that it would be better to focus on the new code, although I would be happy to make the whole code aesthetically ideal.

yaskevich commented 5 years ago

If it is about some specific changes (as converting all the spaces of index.ejs, therefore index.html to tabs), I can easily do that, especially if it could go with next PR.

jonorthwash commented 5 years ago

It's okay to fix spaces and tabs in a file en masse to unify formatting, or to fix formatting here and there to make things easier for you to work with. What I would really like to see is a commit that shows what else you changed. You could revert the commit, manually restore the formatting in the newer version (replace any 4 spaces with tab throughout the file), diff it with the file before the commit, commit that patch, and then fix the formatting in another commit.

yaskevich commented 5 years ago

I've changed these two files (index.ejs and index.html), every 4 spaces were replaced with 1 tab.

jonorthwash commented 5 years ago

What I would really like to see is a commit that shows what else you changed.

Could you at least paste a diff of the new index.ejs and the one before your changes, so I can see what else was changed?

yaskevich commented 5 years ago

I pretty-printed both index.ejs from the original repo and index.ejs from my repo with HTML Formatter: result is here.

Actually, index page, despite it remained functionally ans structurally the same, changed its HTML markup, there was bare HTML initially, and in new version Bootstrap classes are used, which makes the page adaptive.

So, this view 2019-07-23 18_00_47-UD Annotatrix

turned into that:

2019-07-23 18_02_12-UD Annotatrix