-
Notifications
You must be signed in to change notification settings - Fork 177
Add config tab (download configs, filter config, view difference) #964
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
base: dev
Are you sure you want to change the base?
Conversation
|
A preview of de25464 is uploaded and can be seen here: ✨ https://revisit.dev/study/PR964 ✨ Changes may take a few minutes to propagate. |
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.
Pending a few things
- Styling changes
- Copy buttons on the hashes and participant IDs
- NA in time column
- Test whitespace
| Cell: ({ row }: { row: { original: ConfigInfo } }) => ( | ||
| <Flex align="center" gap="xs"> | ||
| <Text> | ||
| {row.original.hash.slice(0, 6)} | ||
| ... | ||
| </Text> | ||
| <Tooltip label={row.original.hash}> | ||
| <IconInfoCircle size={16} /> | ||
| </Tooltip> | ||
| </Flex> | ||
| ), | ||
| }, |
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 also needs a button to copy the value. Can you also please add this copy button to the Participant View ID column?
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.
Added copy button in ConfigView and TableView
| { | ||
| accessorKey: 'timeFrame', | ||
| header: 'Time Frame', | ||
| size: 150, | ||
| }, |
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.
Why are some values in this column NA? Can we do anything better? Maybe start time, not end time, or upload time from the DB?
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.
If no start time or end time is provided, it will use createdTime from the database, which tracks when the participant registered (when they started the study)
/** Time that the participant registered for the study in epoch milliseconds. */
createdTime?: number;| }); | ||
| } | ||
|
|
||
| export function generateDiffView(configs: ConfigInfo[]): JSX.Element | null { |
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 should be in its own component file, not in the utils here
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.
Moved to ConfigDiffViewModal.tsx
| @@ -0,0 +1,255 @@ | |||
| /* eslint-disable react/no-unstable-nested-components */ | |||
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.
Is this required because of the table? Is it simple to remove this?
Does this PR close any open issues?
Closes #246
Closes #416
Give a longer description of what this PR addresses and why it's needed
Provide pictures/videos of the behavior before and after these changes (optional)
Live link: https://revisit.dev/study/PR964/analysis/stats/demo-html/config
Config Tab

Config Filter

Compare Configs

Are there any additional TODOs before this PR is ready to go?
TODOs:
Discussion
Config comparison cannot detect whitespace differences right now.
In
storage/engines/types.tssaveConfig, we hash the config:We use
JSON.stringify, so the config is reformatted (2 space indentation), and the original whitespace and formatting are lost.To add detecting whitespace-only changes, we would need to store the raw config text or we can mention this limitation in the docs?