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

PRO-6561: export documents which are related via rich text widget "internal pages" links and inline images #98

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

Conversation

boutell
Copy link
Member

@boutell boutell commented Dec 2, 2024

Summary

See changelog and title.

What are the specific steps to test this change?

  • See unit tests
  • Can also try the feature (add insert: [ 'image' ] to rich text widget config, add an inline image, add an "Internal Page" link to another page — do not paste a URL, that does not count)

What kind of change does this PR introduce?

(Check at least one)

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Build-related changes
  • Other

Make sure the PR fulfills these requirements:

  • It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
  • The changelog is updated
  • Related documentation has been updated
  • Related tests have been updated

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

Copy link

linear bot commented Dec 2, 2024

@boutell boutell requested a review from ValJed December 2, 2024 19:03
Copy link
Contributor

@ValJed ValJed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor feedback, feature tested it seems working.
Could run cypress tests?

linkedIds.add(linkedDoc.aposDocId);
if (linkedIds.size === 1) {
linkedIdsByType.set(name, linkedIds);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be clearer to check if key exist in map using map has method.:

      for (const linkedDoc of linkedDocs) {
        // Normalization is a little different here because these
        // are individual pages or pieces
        const docType = linkedDoc.slug.startsWith('/')
          ? '@apostrophecms/any-page-type'
          : linkedDoc.type;
        const isTypeStored = linkedIdsByType.has(docType);
        const linkedIds = isTypeStored ? linkedIdsByType.get(docType) : new Set();
        linkedIds.add(linkedDoc.aposDocId);
        if (!isTypeStored) {
          linkedIdsByType.set(name, linkedIds);
        }
      }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

function relatedTypesIncludes(name) {
name = normalizeName(name);
return relatedTypes.includes(name);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's not a big deal here, but if we can take the habit to avoid reassigning stuff like params it would be nice.
Also I know we discussed the fact to introduce a new eslint rule for that, so let's save future refacto time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

storedData,
recursion,
mode
});
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 that in that case schema will always be an empty array?
Maybe we can just return this function getRelatedDocsFromRichTextWidget here since there nothing else to do with it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaving this in place because at least in A2 we eventually created situations where the rich text widget also had a conventional schema. It's at least possible so this is a little more future proof.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok makes sense then

...manager.options.defaultOptions,
...options
};
if ((rteOptions.toolbar?.includes('image') || rteOptions.insert?.includes('image')) && !related.includes('@apostrophecms/image')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please indentation:

      if (
        (rteOptions.toolbar?.includes('image') || rteOptions.insert?.includes('image')) &&
        !related.includes('@apostrophecms/image')
      ) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@boutell boutell requested a review from ValJed December 9, 2024 13:48
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