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

nimble-rich-text-editor and nimble-rich-text-viewer components (out of incubating) #1288

Open
2 tasks done
vivinkrishna-ni opened this issue Jun 8, 2023 · 3 comments
Open
2 tasks done
Labels
client request Client team would immediately benefit from this change enhancement New feature or request new component Request for a new component

Comments

@vivinkrishna-ni
Copy link
Contributor

vivinkrishna-ni commented Jun 8, 2023

😯 Problem to Solve

Requirement for Comments Feature to create, view, and edit rich text content.

Relevant issues:

💁 Proposed Solution

  • FAST component: N/A
  • Other reference components: N/A
  1. nimble-rich-text-editor to create and edit the rich text and get the markdown output.
  2. nimble-rich-text-viewer to view the rich text by providing markdown input

📋 Tasks

  • nimble-rich-text-editor and nimble-rich-text-viewer spec

nimble-rich-text-editor

nimble-rich-text-viewer

Related PRs

Preview Give feedback
@vivinkrishna-ni vivinkrishna-ni added enhancement New feature or request client request Client team would immediately benefit from this change new component Request for a new component labels Jun 8, 2023
This was referenced Jun 8, 2023
@vivinkrishna-ni vivinkrishna-ni changed the title nimble-rich-text-editor and nimble-rich-text-viewer Components nimble-rich-text-editor and nimble-rich-text-viewer components Jun 8, 2023
suseendran-ni pushed a commit that referenced this issue Jun 27, 2023
# Pull Request

## 🤨 Rationale

Creating spec for `nimble-rich-text-editor` and
`nimble-rich-text-viewer` components.
Issue: #1288 

## 👩‍💻 Implementation

This PR contains the nimble spec document for the new nimble component
addition. The spec document consists of a design discussion for the
`nimble-rich-text-editor` and `nimble-rich-text-viewer` components and
explains the external libraries used for the development of a rich text
editor.

## 🧪 Testing

N/A

## ✅ Checklist

N/A
vivinkrishna-ni added a commit that referenced this issue Jul 25, 2023
# 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](https://dev.azure.com/ni/DevCentral/_backlogs/backlog/ASW%20SystemLink%20Platform/Initiatives/?workitem=2426496)

## 👩‍💻 Implementation

* Added
[prosemirror-markdown](https://github.com/ProseMirror/prosemirror-markdown/tree/master)
package for parsing markdown strings, and [prosemirror-model
](https://github.com/ProseMirror/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](https://github.com/markdown-it/markdown-it/tree/master),
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](https://discuss.prosemirror.net/t/unexpected-token-on-importing-of-markdown-stuff/2360/4).
* Also, added `nodePolyfills()` from
[rollup-plugin-polyfill-node](https://www.npmjs.com/package/rollup-plugin-polyfill-node)
as the `markdown-it` requires `punycode`([code
reference](https://github.com/markdown-it/markdown-it/blob/master/lib/index.js#L14))
but as mentioned in the
[punycode](https://www.npmjs.com/package/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

<!--- 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.
vivinkrishna-ni added a commit that referenced this issue Aug 18, 2023
…1416)

# Pull Request

## 🤨 Rationale

This PR introduces the initial implementation of the editor component by
building on top of the `Tiptap` editor. The component is developed to
align with the design for the comments feature and matches the current
visual design linked below.

[Visual
design](https://www.figma.com/file/PO9mFOu5BCl8aJvFchEeuN/Nimble_Components?type=design&node-id=2482-82389&mode=design&t=Kl5FdGYvvpvs9BY8-0)
[Comments feature
mockup](https://www.figma.com/file/Q5SU1OwrnD08keon3zObRX/SystemLink?type=design&node-id=8773-161649&mode=design&t=ZKp2UlDUmvMa56p9-0)

AzDo Feature:
https://dev.azure.com/ni/DevCentral/_backlogs/backlog/ASW%20SystemLink%20LIMS/Features/?workitem=2350963
Issue: #1288

Other functionalities mentioned in the spec document for rich text
editor will be implemented in subsequent PRs.

## 👩‍💻 Implementation

* Installed latest version of
[@tiptap/core](https://www.npmjs.com/package/@tiptap/core) and other
extensions for the supported nodes and marks.
* Updated the spec for installing the actually required packages instead
of `@tiptap/starter-kit`.
* Initialized the editor with the extensions by importing from their
respective packages.
* Added `nimble-toolbar` for the footer section.
* Added `nimble-toggle-button` for the supported formatting options such
as bold, italics, numbered list, and bulleted list and added
functionalities to it. `nimble-toggle-button` uses `click` and `keydown`
events to toggle the state instead of `change` event. The rationale for
adopting this approach is elaborated as follows:
* For more details on the issue, see
#1289 (comment).
* It is due to the fact that the `change` event toggles upon clicking,
whereas the intended behavior is for the state to change only based on
the node that currently holds the cursor focus.
* Since the underlying component of the toggle button functions as a
switch, we must update the checked state in the template and also modify
the handling of change and click events when employing this event
approach.
* On the other hand, in the `click` event approach, we merely reference
it and update the state whenever needed.
* We prototyped both the `change` event and `click` event approach and
decided to use the `click` event as it is simpler to implement. For more
info, refer to the discussion
[here](https://dev.azure.com/ni/DevCentral/_backlogs/backlog/ASW%20SystemLink%20Platform/Initiatives/?workitem=2419268).
* Added `footer-actions` slot element.
* Style the component in a way to match the nimble theme.


![image](https://github.com/ni/nimble/assets/123377523/98c0552c-0e5f-4eca-979c-d5d592772280)

## 🧪 Testing

* Added unit tests and visual sizing tests for the component.
* Manually tested and verified the functionality of the supported
features.

## ✅ 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.

---------
aagash-ni added a commit that referenced this issue Aug 22, 2023
… and get content from Tip Tap editor (#1424)

# Pull Request

## 🤨 Rationale
This PR contains logic for parsing the assigned markdown content,
setting it to the editor and also serializing the editor's data to
markdown content.

AzDo Feature:
https://dev.azure.com/ni/DevCentral/_backlogs/backlog/ASW%20SystemLink%20LIMS/Features/?workitem=2350963
Issue: #1288

## 👩‍💻 Implementation

* Exposed getMarkdown() and setmarkdown() methods to facilitate the
exchange of data with the editor.
* Used MarkdownParser from
[prosemirror-markdown](https://github.com/ProseMirror/prosemirror-markdown/tree/master)
package for parsing markdown strings, DOM serializer from
[prosemirror-model ](https://github.com/ProseMirror/prosemirror-model)
package to serialize the content as DOM structure, XML Serializer to
serialize it to HTML string and sets it to the editor.
* To serialize the tip-tap [document
](https://prosemirror.net/docs/ref/#model.Document_Structure)in
getMarkdown() used MarkdownSerializer from
[prosemirror-markdown](https://github.com/ProseMirror/prosemirror-markdown/tree/master)
package to serialize the node based on schema.
* Enabled `emphasis` and `list` rules in MarkdownParser, this allows
users to set bold, italics, numbered, and bulleted lists in a CommonMark
flavor to the editor. All other basic markdown formats are disabled.
* Added only respective nodes and marks for bold, italics, numbered and
bulleted lists in `MarkdownSerializer`

## 🧪 Testing

* Added unit tests and visual tests for the component.
* Manually tested and verified the functionality of the supported
features.

## ✅ 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.

---------

Signed-off-by: Aagash Raaj <[email protected]>
Co-authored-by: SOLITONTECH\vivin.krishna <[email protected]>
Co-authored-by: SOLITONTECH\aagash.maridas <[email protected]>
Co-authored-by: Jesse Attas <[email protected]>
vivinkrishna-ni added a commit that referenced this issue Aug 31, 2023
# Pull Request

## 🤨 Rationale

This PR includes Angular integration for the `nimble-rich-text-editor`
component by exposing all the necessary APIs planned for the initial
release. See the [API section of the component
spec](https://github.com/ni/nimble/tree/main/packages/nimble-components/src/rich-text-editor/specs#api).

Part of #1288. 

## 👩‍💻 Implementation

- Created Angular directive with input bindings for all APIs
- Added `nimble-rich-text-editor` to the example client app
- Exported module and directive from nimble-angular
- Added page object entry points for `nimble-rich-text-editor` and
`nimble-rich-text-viewer`

## 🧪 Testing

- Created tests for directive APIs 
- Verified the functionalities of all the APIs via the example client
app

## ✅ 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.

---------
rajsite pushed a commit that referenced this issue Sep 6, 2023
# Pull Request

## 🤨 Rationale
Issue Link : #1288

Currently, `rich-text-editor` has `title` and `text content` for
formatting buttons (Bold, Italics, Bulleted List, Numbered List) and
this needs to be localized from the client side.

## 👩‍💻 Implementation

Reference PR for implementation: #1328

This PR  creates label DesignTokens in `nimble-rich-text-editor`

Note: Angular Integration will be taken up once after [this
](#1450 completion.

## 🧪 Testing

- Verified that each label appears in the expected spots in the DOM for
the affected components. Note that the current labels are mostly not
user-visible. Future format button additions will introduce new visible
label tokens.
- Added autotests
- For label providers: verify token naming, and that label provider
attribute updates result in token updates
- Updated
[Storybook](https://60e89457a987cf003efc0a5b-ohnshtynly.chromatic.com/)
- Rich Text Editor has a new "Localizable labels" row in their Storybook
controls/API section
- Under `Tokens/Label Providers` is a new `Rich Text Editor Label
Provider`.

## ✅ 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.

---------

Co-authored-by: Aagash Raaj <[email protected]>
rajsite pushed a commit that referenced this issue Sep 6, 2023
…ich-text-editor (#1477)

# Pull Request

## 🤨 Rationale
Issue Link : #1288

Angular Integration for the rich text component label providers to get
localized from the client app when using the rich text component ( For
now only `rich-text-editor` has labels ).

## 👩‍💻 Implementation

Reference PR for implementation: #1360

This PR adds `NimbleLabelProviderRichTextDirective` in nimble-angular.

## 🧪 Testing

- Added autotests for new directives

## ✅ 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.

---------

Co-authored-by: Aagash Raaj <[email protected]>
@m-akinc
Copy link
Contributor

m-akinc commented Jan 29, 2024

These components now exist in an "incubating" state. This issue now tracks any work necessary to bring it out of incubating.

@m-akinc m-akinc changed the title nimble-rich-text-editor and nimble-rich-text-viewer components nimble-rich-text-editor and nimble-rich-text-viewer components (out of incubating) Jan 29, 2024
@m-akinc m-akinc moved this from In progress to Defined/Ready to Pickup in Nimble Design System Priorities Jan 29, 2024
@jattasNI
Copy link
Contributor

jattasNI commented Oct 1, 2024

We discussed the long term plan to move this out of incubating as part of #2406. The proposal there is that this component belongs in the Spright package within the Nimble repo. It's not a reflection of the quality, completeness, or usefulness of the component, just that it is more special-purpose than typical Nimble components like text field and table.

The work involved would be to move the code from packages/nimble* to packages/spright* and move the stories to the Spright section of storybook. This would be a breaking change for clients as they would now import from spright-angular, SprightBlazor, or spright-components packages. We'd need to either update client apps or notify them to make the updates themselves.

@vivinkrishna-ni What do you think of this proposal? If you like the direction, any chance your team would be interested in implementing that migration?

@vivinkrishna-ni
Copy link
Contributor Author

@jattasNI We have discussed this internally within the team and there are a couple of questions we would like to clarify ourselves before taking up the work of migration for rich text components to Spright. I agree that certain criteria mentioned in the description of this issue have to be met to move it out of incubating.

However, the rich text components might be general-purpose components (Maybe not the viewer, I don't have a strong opinion) that can belong to the core Nimble Design System. So it would be really helpful for us to understand why the components are a special-purpose than typical components in Nimble.

Also, will there be any chance that the components in Spright will be promoted (or is it not applicable?) to Nimble if all the optional requirements for Spright are met as mentioned in the CONTRIBUTING.md?

On the other hand, these components have been in incubation for a long time, as mentioned in #2406, and we agree that these can be moved to Spright as a solution for the issue. I will likely create a work item and link it to our backlog. However, we may not have the bandwidth to prioritize it for this cycle, but I believe we can take it up in the next cycle by considering based on the priorities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client request Client team would immediately benefit from this change enhancement New feature or request new component Request for a new component
Projects
Status: Defined/Ready to Pickup
Development

No branches or pull requests

3 participants