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

Save version history #96

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

Save version history #96

wants to merge 5 commits into from

Conversation

ryansrofe
Copy link
Collaborator

Description

Adds a save to history function when saving FigLogs.

Fixes # (issue)

Type of Change

  • New feature (non-breaking change which adds functionality) 🤞
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Checklist:

  • I have included a changeset if this change will require a version change to one of the packages.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have run all builds, tests, and linting and all checks pass
  • My changes generate no new warnings

ryansrofe added 3 commits May 5, 2024 11:11
lifts up the name and description values
passes to header and changelogs to use in history update
@ryansrofe ryansrofe requested a review from cpresler January 15, 2025 20:14
@ryansrofe
Copy link
Collaborator Author

@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
Copy link
Contributor

@cpresler cpresler left a 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}`;
Copy link
Contributor

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()) {
Copy link
Contributor

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', '');
Copy link
Contributor

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;
Copy link
Contributor

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))
Copy link
Contributor

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.

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