-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Todoist - MVP widget #8
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - the WIP looks good so far 🙂 I've left some comments for you 😉
@@ -38,7 +38,8 @@ | |||
"redux-persist": "6.0.0", | |||
"redux-saga": "1.1.3", | |||
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@@ -36,7 +36,9 @@ const Drawer: React.FC<Props> = ({ addWidgetToLayout }) => { | |||
</Button> | |||
</div> | |||
) | |||
)} | |||
) : | |||
<div className="flex justify-between py-2 italic">{t("widget.common.noWidget")}</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 })); |
@@ -38,7 +38,8 @@ | |||
"redux-persist": "6.0.0", | |||
"redux-saga": "1.1.3", | |||
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
c17932a
to
c323c67
Compare
09c33f0
to
c1bdb40
Compare
WIP - to create the MVP widget for todoist.
Implements: #7