Skip to content
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

feat: add huibizhang todolist #13

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

huibizhang
Copy link

No description provided.

@huibizhang huibizhang changed the title Feat/huibizhang todolist feat: add huibizhang todolist Mar 16, 2022
Copy link
Member

@ashramwen ashramwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

先醬
console 有 warnings 的話要清掉,不然 github action 在 deploy 的時候不會過

WARNING in src/pages/huibizhang/TodoList/TodoList.jsx
  Line 18:9:  'save' is assigned a value but never used  no-unused-vars

src/pages/huibizhang/TodoList/components/PostCreator.jsx
  Line 23:6:  React Hook useEffect has missing dependencies: 'content', 'pin', 'tags', and 'title'. Either include them or remove the dependency array  react-hooks/exhaustive-deps

{
group: '功能',
showGroupName: false,
childs: [{ name: '待辦事項', link: '/' }],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

children

Comment on lines +104 to +109
const current = Object.assign({}, todo);
current.title = postData.title;
current.content = postData.content;
current.pin = postData.pin;
current.tags = postData.tags;
return current;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

現在可以用 spread operator

const current = {
  ...todo,
  title: postData.title,
  content: postData.content,
  pin: postData.pin,
  tags: postData.tags,
};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果 postData 結構一樣的話,甚至可以

const current = { ...todo, ...postData };

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

看了下兩個只有差在 order,所以可以

const { order, ...restPostData } = postData;
const current = { ...todo, ...restPostData };

className="flex aspect-square h-full items-center justify-center rounded-full text-white hover:bg-white/30 dark:text-gray-200"
onClick={() => setDrawerOpened(true)}
>
<svg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

svg 會建議提出成 .svg file

className="xs:grid-cols-2 xs:gap-2 xs:space-y-0 grid grid-cols-1 space-y-2"
>
{posts.pins().map((todo) => {
return <Post key={todo.id} {...todo} open={[edit, setMode]}></Post>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

props 用 array 傳遞,要非常小心順序
我個人不是很常用

{childs.map((child) => {
return (
<label
for="groupItem"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsx 要使用 htmlFor

pin: _pin,
tags: _tags,
order: 0,
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果沒有每個 member 都建立一個 state,而是直接一個物件的話
就可以

addPost({
  id: idGenerator(16),
  order: 0,
  ...editData
});

const idGenerator = (length) => {
const key = ['abcdefghijklmnopqrstuvwxyz', 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', '0123456789'].join('');

var id = '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

別再 var 啦,用 let

tagEditor
? 'bg-gray-300 text-blue-600 dark:bg-gray-800 dark:text-blue-500'
: 'bg-white text-gray-500 dark:bg-gray-700 dark:text-gray-400',
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clsx 的寫法跟其他地方不太一致,建議一個專案都用一種模式

};

useEffect(() => {
setTodos(JSON.parse(window.localStorage.getItem('todos')) ?? [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

這也是可以在建立 state 時就給予初始值

const [drawerOpened, setDrawerOpened] = useState(true);
const [searchText, setSearchText] = useState('');
const [todos, setTodos] = useState([]);
const [mode, setMode] = useState('create');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mode 是不是可以給三種情境

const modalMode = {
  closed: 0,
  create: 1,
  edit: 2,
};

然後 modalOpened 就不需要了?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants