-
Notifications
You must be signed in to change notification settings - Fork 5
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
Save version history #96
base: main
Are you sure you want to change the base?
Conversation
lifts up the name and description values passes to header and changelogs to use in history update
@cpresler can you look at this update and let me know if you think lifting the name and description up a level will actually break older widgets that upgrade? |
also updated typo for depreciated
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.
Over all this looks good, just a few adjustments and it should be 👍
} | ||
|
||
const nameFormatted = nameParts.join(''); | ||
const descriptionFormatted = `${description}\n\n- - - -\n\n${formattedLinks}`; |
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.
I think \n\n- - - -\n\n
should be included in formattedLinks, so if there are no formattedLinks the - - -
doesn't show up.
version?: string, | ||
) { | ||
const formattedLinks = links?.map(link => `${link.label}\n${link.url}`).join('\n\n') || ''; | ||
if (!description.trim() && !formattedLinks.trim()) { |
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.
To be able to add formatting around the links as well, maybe check here against links.length === 0
instead?
Also, why does the lack of description or links stop a version history point from being created? Shouldn't a version history point always be created with a new log, even if it doesn't include a description?
@@ -26,6 +26,9 @@ function Widget() { | |||
// Change Logs | |||
const [changeIds, setChangeIds] = useSyncedState<string[]>('changeKeys', []); | |||
const changeLogs = useSyncedMap<ChangeLog>('changes'); | |||
// Name and Description | |||
const [nameText, setNameText] = useSyncedState('nameText', ''); | |||
const [descriptionText, setDescriptionText] = useSyncedState('descriptionText', ''); |
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.
I tested this with some older widgets and it seems fine.
break; | ||
case 'added': // catch legacy logs with "added" as default type | ||
typeFormatted = ''; | ||
break; |
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.
You don't need to set this to an empty string, it already is one, you can stack the 'none' and 'added' cases together and just break.
Also, I wish we didn't need to repeat this logic since we are using something super similar in the Type component, but not sure if that is worth the refactor at this moment.
showVersion, | ||
version, | ||
) | ||
.then(val => console.log('Version History Saved', val)) |
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.
nit: val
is showing as undefined
in the console every time so it probably isn't worth passing through.
Description
Adds a save to history function when saving FigLogs.
Fixes # (issue)
Type of Change
How Has This Been Tested?
Checklist: