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.

Sign up for GitHub

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

Jump to bottom

Rich text viewer | Implementation of markdown parser #1335

Merged
merged 45 commits into from
Jul 25, 2023
Merged

Rich text viewer | Implementation of markdown parser #1335

merged 45 commits into from
Jul 25, 2023

Conversation

vivinkrishna-ni
Copy link
Contributor

@vivinkrishna-ni vivinkrishna-ni commented Jun 30, 2023 •
edited
Loading

Pull Request

🤨 Rationale

This PR includes the logic for parsing markdown input and rendering the rich text content into the viewer component.
Issue: #1288
AzDo User Story: Show the rich text content by parsing the markdown input

👩‍💻 Implementation

  • Added prosemirror-markdown package for parsing markdown strings, and prosemirror-model package for DOM serializer to serialize the content and render the rich text content in the UI.
  • Added markdownValue accessors to allow setting and retrieving the markdown value. When setting the markdown input string, the component performs operations to convert the markdown string into corresponding document fragments for loading it into the container of the viewer component.
  • Enabled emphasis, list, and autolink from the markdown-it, which is internally used by prosemirror-markdown. This allows users to add bold, italics, numbered, and bulleted lists, as well as direct links in a CommonMark flavor. All other basic markdown formats are disabled.
  • Added json() from the "@rollup/plugin-json" in the rollup.config.js because prosemirror-markdown and markdown-it internally use JSON files during the build process. Referred to this solution from the discussion here.
  • Also, added nodePolyfills() from rollup-plugin-polyfill-node as the markdown-it requires punycode(code reference) but as mentioned in the punycode, the right way to use it is require('punycode/'). So use nodePolyfills() to rollup the bundle with the punycode properly imported in the all-components.bundle.js.

Spec update:

  1. Latest comments desktop and mobile mockup links and updated a few GitHub permalinks.
  2. Removed the fit-to-content attribute for the viewer component.
  3. markdown accessor is changed to a property for the viewer component.

🧪 Testing

Added unit tests and manually tested and verified the changes.

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@vivinkrishna-ni vivinkrishna-ni marked this pull request as draft June 30, 2023 09:50
vikisekarNI
vikisekarNI approved these changes Jun 30, 2023
View reviewed changes
Copy link
Contributor

@vikisekarNI vikisekarNI left a comment

Choose a reason for hiding this comment

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

I am approving this PR to unblock you next week as I will be out of office, otherwise would take another pass.

Pls go through the comments and address them as necessary.

packages/nimble-components/src/rich-text-viewer/index.ts Show resolved Hide resolved
packages/nimble-components/src/rich-text-viewer/index.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/rich-text-viewer/index.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/rich-text-viewer/index.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/rich-text-viewer/testing/rich-text-viewer.pageobject.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/rich-text-viewer/testing/rich-text-viewer.pageobject.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/rich-text-viewer/tests/rich-text-viewer.spec.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/rich-text-viewer/tests/rich-text-viewer.spec.ts Outdated Show resolved Hide resolved
@vivinkrishna-ni
Copy link
Contributor Author

vivinkrishna-ni commented Jun 30, 2023 •
edited by rajsite
Loading

moved to thread: #1335 (comment)

@vivinkrishna-ni vivinkrishna-ni changed the title Users/vivin/viewer markdown parser implementation Rich text viewer | Implementation of markdown parser Jul 1, 2023
@vivinkrishna-ni vivinkrishna-ni mentioned this pull request Jul 5, 2023
Merged
1 task
@vivinkrishna-ni vivinkrishna-ni marked this pull request as ready for review July 7, 2023 12:03
@vivinkrishna-ni vivinkrishna-ni marked this pull request as draft July 10, 2023 07:58
@vivinkrishna-ni vivinkrishna-ni marked this pull request as ready for review July 10, 2023 14:01
@rajsite rajsite mentioned this pull request Jul 18, 2023
Merged
1 task
rajsite added a commit that referenced this pull request Jul 20, 2023
@rajsite @msmithNI
Update dependencies to latest supported (#1361)
0541d6d 
# Pull Request

## 🤨 Rationale

Updates dependencies to the latest resolved by our package.json files
without modifying those specifiers. Hoping it pre-emptively helps the
discussion of [issues around dependency
changes](#1335 (comment))
but useful regardless.

As a bonus, fixes #1146 

## 👩‍💻 Implementation

- Regenerate the lock file (delete lock file, node_modules, and npm
install)
- Update CI node and npm version to reflect Skyline recent update and
better reflect local dev
- Pin Blazor playwright version to correspond to latest released on
nuget.org (as of writing
[1.36.0](https://www.nuget.org/packages/Microsoft.Playwright)) vs latest
released on npm which the previous specifier would resolve to (as of
writing [1.36.1](https://www.npmjs.com/package/playwright)).
- Update tests and stories based on lint rule behavior changes

## 🧪 Testing

Manual test storybook build but rely on CI for most.

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.

---------

Co-authored-by: Malcolm Smith <20709258+msmithNI@users.noreply.github.com>
@vivinkrishna-ni vivinkrishna-ni mentioned this pull request Jul 21, 2023
Open
rajsite
rajsite approved these changes Jul 21, 2023
View reviewed changes
Copy link
Member

@rajsite rajsite left a comment

Choose a reason for hiding this comment

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

Approved assuming default normalization is re-enabled with test cases added

packages/nimble-components/src/rich-text-viewer/tests/rich-text-viewer.spec.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/rich-text-viewer/tests/rich-text-viewer.spec.ts Outdated Show resolved Hide resolved
@jattasNI
Copy link
Contributor

You're good to merge this PR this whenever you're ready! (No need to wait on the last reviewer)

@vivinkrishna-ni vivinkrishna-ni merged commit 05f2975 into main Jul 25, 2023
@vivinkrishna-ni vivinkrishna-ni deleted the users/vivin/viewer-markdown-parser-implementation branch July 25, 2023 03:29
@jattasNI jattasNI mentioned this pull request Dec 4, 2023
Closed
@jattasNI jattasNI mentioned this pull request Jun 17, 2024
Merged
1 task
jattasNI added a commit that referenced this pull request Jun 17, 2024
@jattasNI
Remove node polyfills no longer needed for rich text components (#2195)
4befcd5 
# Pull Request

## 🤨 Rationale

Fixes #1700 These two polyfills were added in #1335 because markdown-it
implicitly depended on some functionality that existed in Node but not
browser environments. We've since upgraded markdown-it to a version that
expresses those dependencies on normal npm packages, so the polyfills
are no longer needed.
markdown-it/markdown-it#967

## 👩‍💻 Implementation

Remove rollup plugins and npm dependencies on those plugins that were
added in #1335.

## 🧪 Testing

Mostly depending on PR build but also spot checking rich text
functionality in storybook.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vikisekarNI vikisekarNI vikisekarNI approved these changes

@suseendran-ni suseendran-ni suseendran-ni approved these changes

@atmgrifter00 atmgrifter00 atmgrifter00 approved these changes

@jattasNI jattasNI jattasNI approved these changes

@rajsite rajsite rajsite approved these changes

@mollykreis mollykreis Awaiting requested review from mollykreis mollykreis is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vivinkrishna-ni @jattasNI @rajsite @atmgrifter00 @vikisekarNI @suseendran-ni