Second step : Category hierarchy & article visibility - [merged] #44
Comments
|
In GitLab by @adrien.schiehle on Sep 24, 2021, 12:24 added 1 commit
|
|
In GitLab by @adrien.schiehle on Sep 24, 2021, 12:26 Security has been added to avoid infinite loops
|
|
requested review from @aaclayton |
|
Are there parameters that are supported aside from async getCategories({limit=50, offset=0}={}) {
...
} |
|
We should request with Dimitris that the API explicitly return a key for whether more results exist or not. As it stands it can be the case that there are exactly 50 categories, and so we don't know whether there are more to fetch or not. This applies to the articles API also. |
|
Should this be |
|
Rather than documenting as type |
|
Prefer using |
|
Please declare all functions as |
|
I would like the |
|
We should refactor these control buttons to have a shared handler that delegates actions. The pattern I would like to use is this: HTML:* <button type="button" class="world-anvil-control" data-action="toggle-wip"></button>JS: // In activateListeners
html.find(".world-anvil-control").click(this._onClickControlButton.bind(this));
// Below
_onClickControlButton(event) {
event.preventDefault();
const button = event.currentTarget;
const action = button.dataset.action;
switch ( action ) {
case "toggle-wip"
break;
// other actions
}
} |
|
Hey Adrien, there is a lot here, so my first round of feedback is focused on the overall code structure and design. Once you get a chance to iterate on these comments I'll do a subsequent round(s) of feedback on the internal logic. |
|
removed review request for @aaclayton |
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:03 Commented on module/api.js line 146 We directly pass them to the WA api. Currently we only use the default values. I agree with you on saying default parameters is better. I will also change it for getArticles(). (Which is what I copied from 👼 ) |
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:03 Commented on module/api.js line 146 changed this line in version 3 of the diff |
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:03 Commented on module/api.js line 149 changed this line in version 3 of the diff |
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:03 added 1 commit
|
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:07 Commented on module/api.js line 149 I didn't dwell too much in this. I just used the same method which was used for getArticles(). Maybe there already is some data on the batch object, I will look into it. |
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:09 Commented on module/api.js line 152 changed this line in version 4 of the diff |
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:09 added 1 commit
|
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:09 Commented on module/api.js line 152
I didn't have 50 categories for my tests, didn't see it. |
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:20 Commented on module/journal.js line 8 changed this line in version 5 of the diff |
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:20 added 1 commit
|
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:20 Commented on module/journal.js line 8 I have been a little lenient on those ones... I will add basic type for them. I come from the backend side, with strong types. I was a little lost in the beginning when discovering javascript. |
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:27 Commented on module/journal.js line 28 changed this line in version 6 of the diff |
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:27 added 1 commit
|
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:27 Commented on module/journal.js line 28 I changed it. May I ask it to appear in release note if you chose to remove it on 0.9 or 0.10 ? I find this method really useful, and I tend to use it almost everywhere in my custom code... |
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:33 Commented on module/journal.js line 14 changed this line in version 7 of the diff |
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:33 added 1 commit
|
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:33 Commented on module/journal.js line 14 I never really understood the differences between the two of them (even after reading post on it) If you want to only use function inside module, that's ok for me. I will do the change. |
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:40 Commented on module/journal.js line 4 Hmmm... I think framework.js already do the work of your utilities.js file.
We could perhaps put the function (This is a two lines function that call importArticle after trying to retrieve it from the Journal entries) |
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 12:55 Commented on module/journal.js line 324 I'm really curious for this one. I'm mostly self taught in frontend dev, and I may have picked really bad habits. When I have buttons with similar display but different behavior, I tend to use something like that : <button type="button" class="css-class action-class"></button>And after that .wy-world .my-sheet .css-class {
/*stuff here */
}html.find(".action-class").click(this._onClickDoSomething.bind(this));It allows me to have :
I find it more easy to read, and to describe inside comments
You previously put a one somewhat big method in journal.js that was handling all the clicks. It was manageable with the number of clicks you had. But now with the visibility buttons (and later, the secrets one), it became far too big. That's why I splitted it, and put action-classes instead of data-action. If the problems comes from the html.find("button.link-folder").click(this._onClickLinkFolder.bind(this));
html.find("button.display-folder").click(this._onClickDisplayFolder.bind(this));
html.find("button.hide-folder").click(this._onClickHideFolder.bind(this));
html.find("button.sync-folder").click(this._onClickSyncFolder.bind(this));
html.find("button.link-entry").click(this._onClickLinkEntry.bind(this));
html.find("button.browse-entry").click(this._onClickBrowseEntry.bind(this));
html.find("button.display-entry").click(this._onClickDisplayEntry.bind(this));
html.find("button.hide-entry").click(this._onClickHideEntry.bind(this));
html.find("button.toggle-drafts").click(this._onClickToggleDrafts.bind(this));
html.find("button.toggle-wip").click(this._onClickToggleWIP.bind(this)); |
|
In GitLab by @adrien.schiehle on Sep 27, 2021, 13:06 Commented on module/api.js line 149 Unfortunately, nothing on the batch object. I tried to limit it to five element, so that I can check if there were something new retrieved, but no 😓 {
"world": { "id": "d219435f...", [...]},
"term": null,
"offset": 0,
"limit": "5",
"order_by": "id",
"trajectory": "ASC",
"categories": [ {...}, {...}, {...}, {...}, {...} ]
}It's the same structure as the one used from getArticles(). You think Dimitris will add something here? It's not the best, but its pretty standard and works. I vote for letting it as it is. |
|
requested review from @aaclayton |
|
Thanks for your patience here, haven't forgotten about this one - just a bit overwhelmed right now with other work. |
|
In GitLab by @adrien.schiehle on Oct 12, 2021, 13:01 Commented on module/journal.js line 324 changed this line in version 8 of the diff |
|
In GitLab by @adrien.schiehle on Oct 12, 2021, 13:01 added 4 commits
|
|
In GitLab by @adrien.schiehle on Oct 12, 2021, 13:03 Commented on module/journal.js line 324 I put it back as it was since you don't have much time for now to do a big review |
|
In GitLab by @adrien.schiehle on Oct 12, 2021, 13:07 added 1 commit
|
|
changed target branch from |
In GitLab by @adrien.schiehle on Sep 24, 2021, 12:17
Merges categories -> categories
A little bigger one. It has some recursive code to go through the whole category tree.
But that shouldn't pose any problem as long as there is no infinite loop in what we retrieve for World Anvil.
This is what it does :
The text was updated successfully, but these errors were encountered: