-
Notifications
You must be signed in to change notification settings - Fork 379
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
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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}> | |||
|
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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?
Hi @BDhara, still interested in keeping the PR? Please let us know if you need any help with the comments that Alex provided |
Hi @BDhara, Are you still working on this one? Feel free to reach out if you need assistance / guidance 🙂 |
Hi Michael,
I will be working on this PR in short time.
Regards,
Dhara Bhatia
…On Thu, Apr 25, 2024, 10:46 AM Michaël Maillot ***@***.***> wrote:
Hi @BDhara <https://github.com/BDhara>,
Are you still working on this one? Feel free to reach out if you need
assistance / guidance 🙂
—
Reply to this email directly, view it on GitHub
<#1668 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACTTB33HJRZOHL52HQGTHTDY7ECK7AVCNFSM6AAAAAA5P2FD56VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZXGIZTINBVGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi @BDhara, Sorry to get back to you on this one, but are still working on it? |
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.