Todoist - MVP widget#8
Conversation
darekkay
left a comment
darekkay
left a comment
There was a problem hiding this comment.
Thanks - the WIP looks good so far 🙂 I've left some comments for you 😉
| "redux-sagas-injector": "1.1.1", | ||
| "selectorator": "4.0.3" | ||
| "selectorator": "4.0.3", | ||
| "todoist-rest-api": "^1.3.4" |
There was a problem hiding this comment.
Please use exact package versions to make sure that all instances (development, production, CI) use exactly the same versions:
| "todoist-rest-api": "^1.3.4" | |
| "todoist-rest-api": "1.3.4" |
With yarn, you can enforce this with yarn add package-name --exact (or -E), with npm it's npm install package-name --save --save-exact.
| ) | ||
| )} | ||
| ) : | ||
| <div className="flex justify-between py-2 italic">{t("widget.common.noWidget")}</div> |
There was a problem hiding this comment.
I think we shouldn't use a category that has no widgets (that's why the unused are ones commented out). This way, this change (and the according label) are unnecessary. That said, the widget drawer requires a major redesign anyway.
| export const initialHeight = 2; | ||
| export const initialWidth = 3; | ||
| export const initialOptions = { | ||
| token: "xxxxx" |
There was a problem hiding this comment.
The token should be empty or undefined by default. That way, you can use <WidgetUnconfigured> as long as the token is missing (see the usage in other widgets).
| initialHeight: 3, | ||
| initialWidth: 4, | ||
| initialOptions: {}, | ||
| initialMeta: { updateCycle: { hours: 24 } } |
There was a problem hiding this comment.
This file is autogenerated. Please run yarn scan-widgets after any change to properties.ts. Currently the values are ouf of sync.
I've looked into ways of improving this workflow by using babel macros, but without ejecting from create-react-app (or forking it), it doesn't get much better.
| }); | ||
|
|
||
| it("renders without error", () => { | ||
| expect(wrapper.isEmptyRender()).toBe(false); |
There was a problem hiding this comment.
FYI - the default (= generated) tests are currently very basic, because I'm planning to replace enzyme with react-testing-library in the future. If you're experienced with react-testing-library (which I'm not that much, yet 😅 ), feel free to write some basic tests. Otherwise you can ignore it for now 😉
| log.error(error); | ||
| yield put(updateError(id)); |
There was a problem hiding this comment.
Please merge/rebase latest master into your branch - I've changed this yesterday a little bit to improve the client-side error handling. After merging/rebasing, this change is required:
| log.error(error); | |
| yield put(updateError(id)); | |
| yield put(updateError({ id, error })); |
| "redux-sagas-injector": "1.1.1", | ||
| "selectorator": "4.0.3" | ||
| "selectorator": "4.0.3", | ||
| "todoist-rest-api": "^1.3.4" |
There was a problem hiding this comment.
A second, more generic comment: I've started the project without any server at all, thinking I could go as far as possible with that approach. However, I've now implemented the first 3rd-party-data-driven widgets (Cryptocurrencies, GitHub Stats). Now I think we should handle all 3rd-party data via the server module for the following reasons:
-
Request caching. If the 3rd-party request is implemented on the client, the Todoist API will be hit directly every time the user refreshes the page (page refresh overrides the widget update cycle).
-
When we switch to OAuth later, a developer key will be required. If the request doesn't go through the server module, the developer key will be exposed to the users, which it shouldn't.
In other words, I would move this module from app/package.json to server/package.json and create internal /todoist route(s) as a proxy/facade.
The server module is only a few weeks old, but there are already 3 different routes implemented (2 of them actually used) that you can check as an example.
darekkay
force-pushed
the
master
branch
3 times, most recently
from
c17932a to
c323c67
Compare
darekkay
force-pushed
the
master
branch
2 times, most recently
from
09c33f0 to
c1bdb40
Compare
2 participants
WIP - to create the MVP widget for todoist.
Implements: #7