Conversation
tessalt
force-pushed
the
feature/CORL-547
branch
from
21446c6 to
33774b5
Compare
alinhle
left a comment
alinhle
left a comment
There was a problem hiding this comment.
I have some concerns about the implementation of the announcement config as it conflicts with the top level form save button which makes it feel unintuitive and foreign. Let's discuss.
| hour: "2-digit", | ||
| minute: "2-digit", | ||
| }).format(new Date(disableAt)); | ||
| }, [disableAt]); |
There was a problem hiding this comment.
This looks like disableAt changing everytime which defeats the purpose of useMemo. I think line 26-28 should be inside the callback as well.
| settings={settings} | ||
| createAnnouncement={createAnnouncement} | ||
| deleteAnnouncement={deleteAnnouncement} | ||
| /> |
There was a problem hiding this comment.
A small suggestion would be to combine AnnouncementConfigContainer.tsx and AnnouncementConfig.tsx together into AnnouncementConfigContainer.tsx.
| return [storedValue, setValue]; | ||
| } | ||
|
|
||
| export default useLocalStorage; |
There was a problem hiding this comment.
We have abstracted localStorage into CoralContext: src/core/client/framework/lib/bootstrap/CoralContext.tsx, so we should probably reuse that.
| form: FormApi | ||
| ) => { | ||
| await this.props.updateSettings({ settings: data }); | ||
| const payload = omit(data, "announcement"); |
There was a problem hiding this comment.
I'm a bit worried about this, let's have a chat tomorrow.
There was a problem hiding this comment.
we had a chat about this in #coral-tech-talk a few days ago and couldn't come up with a good solution that didn't involve re-architecting the whole configuration section. Love to hear your suggestions on this!
tessalt
force-pushed
the
feature/CORL-547
branch
from
2a74628 to
7c1f94d
Compare
| useEffect(() => { | ||
| async function getDismissedStatus() { | ||
| if (!settings.announcement) { | ||
| setDismissed(true); |
There was a problem hiding this comment.
You can just ignore changing state when settings.announcement is not set right, because it's rendered as null anyway.
alinhle
left a comment
alinhle
left a comment
There was a problem hiding this comment.
One last comment: #2813 (comment).
Otherwise looks good, great work!
|
Maybe add tests if you feel like. |
3 participants
What does this PR do?
How do I test this PR?