-
Notifications
You must be signed in to change notification settings - Fork 38
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/link-audit frontend #1949
feat/link-audit frontend #1949
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.
generally lgtm, just mostly some nits!
would like to ask for your opinions on state management for link history - personally it seems a bit overkill to put the state for link history in the global redux store under UserState
when it could be local to the LinkHistory
component, but then there's also already an established pattern in the codebase of putting lots of things under the redux store. would it be better to follow this redux approach or try using a more local state here?
edit: also I just remembered something you mentioned before, where loading a new link history would temporarily display the previous stale link history before the new ones are fetched - local state should be able to help with that too by 'purging' link history state when unmounting the component
edit 2: also just saw feli's comments on figma regarding alignment - would the timeline alignment be something doable too?
src/client/user/components/Drawer/ControlPanel/LinkHistory/LinkHistory.tsx
Outdated
Show resolved
Hide resolved
src/client/user/components/Drawer/ControlPanel/LinkHistory/LinkHistoryButton.tsx
Outdated
Show resolved
Hide resolved
src/client/user/components/Drawer/ControlPanel/LinkHistory/LinkHistoryPagination.tsx
Outdated
Show resolved
Hide resolved
@halfwhole Thank you for the review! Amended most of them but would discuss with you in the office tmr about the changes related to the redux store |
following a convo with @jimvae, we're good with using redux and following the current style for now - we can refactor the frontend to use other state management techniques another time! |
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.
lgtm!
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.
lgtm!
Problem
As a user, I want to know what has changed on my link and by whom. This includes
Link Status
,Link Owner
,Long URL
andFile Upload
changes.Relevant Links:
Notion story - User story and acceptance criteria
Backend - Backend tasks for this feature
Solution
user
redux states for link-audit (isFetchingLinkHistory
,linkHistory
andlinkHistoryCount
) for fetching link-audit informationtoggleLinkHistory
) to facilitate toggling between link history and link details in the link drawerScreenshots
View Link History Button
that goes from link details to link historyLink History Timeline
to showcase different changesPagination
End to End Tests
Link Status
Link Owner
Deploy Notes
New dependencies:
"@material-ui/lab": "^4.0.0-alpha.61",
: added this to use the Timeline componentWhen this dependency is updated in the future, there is a possibility that they shift some of the components to material-ui/core. This might lead to import errors which are easily fixable by changing the import source (easily caught during compilation)