-
Notifications
You must be signed in to change notification settings - Fork 44
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
Editor link icon open modal #9220
Editor link icon open modal #9220
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.
@MuhammadKaleem171 Looks good and works very well! I just asked for some small changes. I'm approving this now but please try to fix the failing CI/CD tests before trying to merge. If you're unable to see the details of why it's failing just let me know. Also for any future PRs, please assign me and @mzparacha @masvelio and @burtonator as reviewers, as we require at least 2 approvals from other engineers for the PR to be merged.
packages/commonwealth/client/scripts/views/components/react_quill_editor/react_quill_editor.tsx
Show resolved
Hide resolved
packages/commonwealth/client/scripts/views/components/react_quill_editor/QuillLinkModal.tsx
Outdated
Show resolved
Hide resolved
"@Israellund I'm trying to troubleshoot a CI/CD error and would appreciate your help in understanding the issue. Could you assist me in identifying and resolving the problem?" |
@MuhammadKaleem171 Of course. Are you able to view the CI/CD error logs? If not I am attaching screenshots below of the EVM Devnet test errors. Please let me know if you need anything else |
packages/commonwealth/client/scripts/views/components/react_quill_editor/QuillLinkModal.tsx
Outdated
Show resolved
Hide resolved
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.
it works well but please adjust the code according to my comments. It will be crucial to be consistent with existing codebase, just to prevent mess. Thanks for contribution!
packages/commonwealth/client/scripts/views/components/react_quill_editor/QuillLinkModal.tsx
Outdated
Show resolved
Hide resolved
packages/commonwealth/client/scripts/views/components/react_quill_editor/react_quill_editor.tsx
Outdated
Show resolved
Hide resolved
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 changes, please follow my advices in the future. Good start!
@masvelio @Israellund should i close the conversation ? |
@MuhammadKaleem171 thanks for giving acknowledge to the specific comments that you fixed the issue. If it not requires further discussion, you can resolve it. |
…to 8553.kaleem.editor-link-icon
Editor link icon should open modal
Closes: #8553
Description of Changes
"How We Fixed It"
Created the new modal and added two input for text and link .