Fix issue #158 #215
Fix issue #158 #215
Conversation
| this.platform = options.platform; | ||
| } | ||
| }, | ||
| render: function() { | ||
| this.$el.html(L.templates["pages/help"]()); | ||
|
|
||
| if(this.platform) { | ||
| if(this.platform === "firefox" || this.platform === "chrome") { |
There was a problem hiding this comment.
Platform specific code and checks do not belong in the core code base.
There was a problem hiding this comment.
So do you have any suggestions for handling the situation? For example, the additional help methods only appear in "firefox" version and "chrome" version, and the warning-persistence only appear in "webapp" version. Should I create the same file for every platform even if some of them are empty?
There was a problem hiding this comment.
I think I see what you are doing here. No, we shouldn't create the same files for every platform. Instead, we should remove any this.platform checks (don't even pass the parameter) and simply see if L.templates["help"] is defined (although we should probably rename the template to platform-help). If it is defined, platform specific help text exists and should be rendered.
In general, code that checks for specific platforms should never go in the core code-base as they will inevitably lead to bugs (we'll forget to extend a check when implementing a new platform and things will break).
|
If these three commits are addressing separate issues, it's generally If these three commits are all addressing the same issue, it may be
|
|
They are actually addressing 4 separate issues. 213b09a should be broken up into two commits. However, while nice, separate pull requests aren't really necessary; the request simply won't be merged until everything is fixed. |
| return this; | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
Exactly. However, the login state might change so you need to listen on changes to the registered attribute of L.server and update accordingly. Also, there's really no point in putting the if statement into the template, just put it outside the template and only render the warning if L.server.get('registered') returns false. This saves rendering an unused template.
|
Delete all the platform-specific code, make the login popup listening to the registered state of the server, and rebase small commits into one big commit. |
|
I see 7 commits... |
|
That is because I already pushed some commits before and could not delete them remotely. Also when I rebase, I have to merge with the remote git repo. All the changes are in the commit xiangcy@d9735d1 |
Logical CommitsKeep it to one commit per meaningful step. In this case, you need two commits, one for each issue. Preferably, these commits would be in two separate pull requests in this case that's up to you. Basically, if you have:
You should have:
However, you shouldn't have:
Rebasing
You don't. When finalizing a pull request, do the following:
You don't really need to (and probably shouldn't) do this when simply revising a pull request (due to feedback) because it destroys history. However, when the pull request is ready to merge, I'll ask you to rebase and rework your commits so that the master history is clean. |
| L.views.MainPage = L.views.MainPage.extend({ | ||
| initialize: function() { | ||
| OldMainPage.prototype.initialize.call(this); | ||
| //this.listenTo(L.server, 'change:registered', this.togglePopUpAlert()); |
There was a problem hiding this comment.
Did you forget to uncomment this?
|
Add a class "not-logged-in" for the main page, show the login warning when not logged in. |
| if(!L.server.get('registered')){ | ||
| this.$el.addClass('not-logged-in'); | ||
| } else { | ||
| this.$el.removeClass('not-logged-in'); |
There was a problem hiding this comment.
Shorter:
this.$el.toggleClass('not-logged-in', !L.server.get('registered'))
|
Nice! I'll merge it after you address my nitpick and rebase into two commits (one for each bug). |
|
Done. Fix the last thing and rebase into one commit only for bug 158. |
|
There are still two changes here: the platform specific help changes and the persistence warning. These should be in two separate commits. |
|
Decouple into two separate commits. Wait for merging to the main branch! |
No description provided.