Skip to content

Todoist - MVP widget #8

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ package-lock.json
/.idea/workspace.xml
/.idea/tasks.xml
/.idea/shelf
*.code-workspace

### Misc ###

*.log
.DS_Store
.DS_Store
3 changes: 2 additions & 1 deletion app/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Owner

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:

Suggested change
"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.

Copy link
Owner

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:

  1. 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).

  2. 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.

},
"devDependencies": {
"@storybook/addon-a11y": "5.3.18",
Expand Down
13 changes: 11 additions & 2 deletions app/src/common/translations/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,17 @@
"add": "Add {{widget}} widget",
"remove": "Remove {{widget}} widget",
"configuration": "Configuration: {{widget}}",
"unconfigured": "Widget configuration required."
"unconfigured": "Widget configuration required.",
"noWidget": "No widget available"
},
"category": {
"general": "General",
"media": "Media",
"knowledge": "Knowledge",
"tools": "Tools",
"tracking": "Tracking",
"monitoring": "Monitoring"
"monitoring": "Monitoring",
"productivity": "Productivity"
},

"counter": {
Expand Down Expand Up @@ -133,6 +135,13 @@
"name": "Notes",
"headline": ""
},
"todoist": {
"name": "Todoist",
"headline": "Todoist",
"configuration" : {
"token": "Token"
}
},
"totd-chemical-elements": {
"name": "Chemical elements",
"headline": ""
Expand Down
6 changes: 4 additions & 2 deletions app/src/components/drawer/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const Drawer: React.FC<Props> = ({ addWidgetToLayout }) => {
<div className="p-2 text-center uppercase font-bold text-2">
{t(`widget.category.${category}`)}
</div>
{categoriesWithWidgets[category].map(
{categoriesWithWidgets[category] ? categoriesWithWidgets[category].map(
({ widgetType: widget }: WidgetProperties) => (
<div key={widget} className="flex justify-between py-2">
{t(`widget.${widget}.name`)}
Expand All @@ -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>
Copy link
Owner

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.

}
</div>
))}
</div>
Expand Down
7 changes: 4 additions & 3 deletions app/src/widgets/categories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ enum WidgetCategory {
Knowledge = "knowledge",
Tools = "tools",
Tracking = "tracking",
Monitoring = "monitoring"
Monitoring = "monitoring",
Productivity = "productivity"

// Possible categories for future widgets
// Charts = "charts",
// Productivity = "productivity",
// Motivation = "motivation",
// Fun = "fun",
// Games = "games"
Expand All @@ -20,7 +20,8 @@ export const categories = [
WidgetCategory.Knowledge,
WidgetCategory.Tools,
WidgetCategory.Tracking,
WidgetCategory.Monitoring
WidgetCategory.Monitoring,
WidgetCategory.Productivity
];

export default WidgetCategory;
9 changes: 9 additions & 0 deletions app/src/widgets/list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,15 @@ export default {
initialOptions: {},
initialMeta: {}
},
todoist: {
configurable: true,
widgetType: "todoist",
category: "productivity",
initialHeight: 3,
initialWidth: 4,
initialOptions: {},
initialMeta: { updateCycle: { hours: 24 } }
Copy link
Owner

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.

},
"totd-chemical-elements": {
configurable: false,
widgetType: "totd-chemical-elements",
Expand Down
20 changes: 20 additions & 0 deletions app/src/widgets/todoist/__stories__/index.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import React from "react";
import { storiesOf } from "@storybook/react";

import { connectedWidgetProps } from "common/utils/mock";
import { Widget } from "components/widget";

const Story = () => {
return (
<Widget
{...connectedWidgetProps}
id="todoist-mock-id"
type="todoist"
data={{
token: "xxxx"
}}
/>
);
};

storiesOf("Widgets.Todoist", module).add("Variants", () => <Story />);
24 changes: 24 additions & 0 deletions app/src/widgets/todoist/__tests__/index.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import React from "react";
import { shallow, ShallowWrapper } from "enzyme";

import { widgetProps } from "common/utils/mock";

import Todoist from "../index";

describe("<Todoist />", () => {
let wrapper: ShallowWrapper;

beforeEach(() => {
wrapper = shallow(
<Todoist
{...widgetProps}
id="todoist-mock-id"
token="xxxx"
/>
);
});

it("renders without error", () => {
expect(wrapper.isEmptyRender()).toBe(false);
Copy link
Owner

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 😉

});
});
26 changes: 26 additions & 0 deletions app/src/widgets/todoist/configuration.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import React from "react";
import { useTranslation } from "react-i18next";

import { ConfigurationProps } from "widgets/index";
import Input from "components/forms/input";

const Configuration = ({
id,
options,
setOptions,
save
}: ConfigurationProps) => {
const { t } = useTranslation();
return (
<>
<Input
label={t("widget.todoist.configuration.token")}
className="mb-6"
value={options.token}
setValue={value => setOptions({ token: value })}
></Input>
</>
);
};

export default Configuration;
36 changes: 36 additions & 0 deletions app/src/widgets/todoist/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import React, { memo } from "react";

import useTriggerUpdate from "common/hooks/useTriggerUpdate";

import { WidgetProps } from "../index";

export { saga } from "./sagas";

const Todoist: React.FC<Props> = memo(
({
id,
token,
meta,
setData,
triggerUpdate
}) => {
useTriggerUpdate({ id, params: { token }, meta, triggerUpdate }, [
token
]);

return (
<div className="flex flex-col items-center text-center">
<div className="text-4 font-semibold">
<span className="center"> Todoist </span>
<span className="text-2 uppercase">{token}</span>
</div>
</div>
);
}
);

interface Props extends WidgetProps {
token: string;
}

export default Todoist;
12 changes: 12 additions & 0 deletions app/src/widgets/todoist/properties.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import WidgetCategory from "../categories";

export const widgetType = "todoist";
export const category = WidgetCategory.Productivity;
export const initialHeight = 2;
export const initialWidth = 3;
export const initialOptions = {
token: "xxxxx"
Copy link
Owner

@darekkay darekkay Apr 23, 2020

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).

};
export const initialMeta = {
updateCycle: { minutes: 15 }
};
42 changes: 42 additions & 0 deletions app/src/widgets/todoist/sagas.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { put, call, takeEvery } from "@redux-saga/core/effects";
import { PayloadAction } from "@reduxjs/toolkit";

import log from "common/log";
import api, { CRYPTOCURRENCIES_PRICE } from "common/api";
import {
setData,
triggerUpdate,
updatePending,
updateError,
updateSuccess,
TriggerUpdateAction
} from "components/widget/duck";

import { widgetType } from "./properties";

const fetchCryptocurrencyPrice = async (params: { [key: string]: any }) => {
const response = await api.get(CRYPTOCURRENCIES_PRICE, { params });
return response.data;
};

function* onTriggerUpdate(action: PayloadAction<TriggerUpdateAction>) {
const { id, params } = action.payload;
yield put(updatePending(id));
try {
const data = yield call(fetchCryptocurrencyPrice, params);
yield put(
setData({
id,
values: data
})
);
yield put(updateSuccess(id));
} catch (error) {
log.error(error);
yield put(updateError(id));
Comment on lines +35 to +36
Copy link
Owner

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:

Suggested change
log.error(error);
yield put(updateError(id));
yield put(updateError({ id, error }));

}
}

export function* saga() {
yield takeEvery(triggerUpdate(widgetType).toString(), onTriggerUpdate);
}
Loading