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

Fix/add new tab option rich text #1668

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

BDhara
Copy link

@BDhara BDhara commented Oct 2, 2023

Q A
Bug fix? [ ]
New feature? [x]
New sample? [ ]
Related issues? fixes #X, partially #Y, mentioned in #Z

What's in this Pull Request?

Introduce new feature in Rich text link - display link target with options Same tab / New tab. By default Same tab.
image
image

Copy link
Collaborator

@AJIXuMuK AJIXuMuK left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @BDhara!

I have reviewed it and provided some comments.

@@ -97,6 +97,19 @@
color: "[theme:neutralLighterAlt, default:#{$ms-color-neutralLighterAlt}]";
}

.toolbarSubmenuCaretLT {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need this non-standard caret style for the dropdown?
Why don't just use a default one?

@@ -89,6 +89,15 @@ export class RichText extends React.Component<IRichTextProps, IRichTextState> {
text: strings.ListNumbered,
data: { icon: 'NumberedList' }
}];
private ddLinkTargetOpts = [{
key: '_self',
id: 'same',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd just use the same _self value for id. Don't see a reason to have some custom non-standard name here. Or remove the id completely.

@@ -394,6 +404,15 @@ export class RichText extends React.Component<IRichTextProps, IRichTextState> {
}
}} />

<Dropdown
id="DropDownLinkTarget"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, do not provide ids for the elements.

@@ -1001,6 +1060,7 @@ export class RichText extends React.Component<IRichTextProps, IRichTextState> {
if (label) {
return (
<Label htmlFor={this._richTextId}>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, remove this empty line

@@ -3,7 +3,7 @@ import * as strings from 'ControlStrings';
import 'react-quill/dist/quill.snow.css';
import RichTextPropertyPane from './RichTextPropertyPane';
import ReactQuill, { Quill as ReactQuillInstance } from 'react-quill';
import type { Quill } from 'quill';
import { Quill } from 'quill';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you remove type from the import?

@joelfmrodrigues
Copy link
Collaborator

Hi @BDhara, still interested in keeping the PR? Please let us know if you need any help with the comments that Alex provided

@michaelmaillot
Copy link
Collaborator

Hi @BDhara,

Are you still working on this one? Feel free to reach out if you need assistance / guidance 🙂

@BDhara
Copy link
Author

BDhara commented Apr 25, 2024 via email

@michaelmaillot
Copy link
Collaborator

Hi @BDhara,

Sorry to get back to you on this one, but are still working on it?

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.

4 participants