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(rich-text-editor): NO-JIRA multiple improvements #402

Merged
merged 28 commits into from
Jul 12, 2024

Conversation

braddialpad
Copy link
Contributor

fix(rich-text-editor): multiple improvements

Obligatory GIF (super important!)

Obligatory GIF

🛠️ Type Of Change

These types will increment the version number on release:

  • Fix

📖 Jira Ticket

https://dialpad.atlassian.net/browse/DLT-1825
https://dialpad.atlassian.net/browse/DLT-1858
https://dialpad.atlassian.net/browse/DLT-1856
https://dialpad.atlassian.net/browse/DLT-1674

📖 Description

Fixes for multiple reported issues with rich-text-editor / message-input

  • DLT-1825: prevent the triggering of keydown events in suggestion popover when the item count is 0, this was causing multiple errors
  • DLT-1858, DLT-1856: trigger enter event from the input field directly rather than the input wrapper.
  • DLT-1674: Designer reported UI improvements. Lots of these will be fixed when nina's scrollbar PR gets merged. I just changed the button colors as requested.

💡 Context

Reported issues from the implementation of message input into dialpad.

📝 Checklist

For all PRs:

  • I have ensured no private Dialpad links or info are in the code or pull request description (Dialtone is a public repo!).
  • I have reviewed my changes.
  • I have added all relevant documentation.
  • I have considered the performance impact of my change.

For all Vue changes:

  • I have made my changes in Vue 2 and Vue 3. Note: you may sync your changes from Vue 2 to Vue 3 (or vice versa) using the ./scripts/dialtone-vue-sync.sh script. Read docs here: Dialtone Vue Sync Script

For all CSS changes:

  • I have used design tokens whenever possible.

🔮 Next Steps

Notify the messaging team that these have been fixed.

Copy link

github-actions bot commented Jul 9, 2024

Please add either the visual-test-ready or no-visual-test label to this PR depending on whether you want to run visual tests or not.
It is recommended to run visual tests if your PR changes any UI. ‼️

@braddialpad braddialpad changed the title fix(rich-text-editor): multiple improvements fix(rich-text-editor): NO-JIRA multiple improvements Jul 9, 2024
@braddialpad braddialpad added the visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests label Jul 9, 2024
@iropolo
Copy link
Contributor

iropolo commented Jul 10, 2024

On Conversation View -> Message input -> Default vue2 if you press enter, it doesn't add a jump line, it does it in vue3

Screen.Recording.2024-07-10.at.11.59.37.mov

Non on vue3 if you press arrow down key it trigger console error.

Screen.Recording.2024-07-10.at.11.58.03.mov

Copy link
Contributor

@iropolo iropolo left a comment

Choose a reason for hiding this comment

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

Apart from my comments, fixs looks good

@braddialpad
Copy link
Contributor Author

On Conversation View -> Message input -> Default vue2 if you press enter, it doesn't add a jump line, it does it in vue3

It's actually only supposed to add a line if you use "shift + enter" pressing enter normally would send the message. I will look into this.

@braddialpad
Copy link
Contributor Author

Couple more small requested fixes that came in from design today that I will add and document.

@braddialpad
Copy link
Contributor Author

Added:

  • tertiary foreground color to slash command subtitle
  • moved max-height to the dialog rather than the child container, so it is retained when scrolling to bottom.
  • slot for "top" so they can add custom messages above the input field, but still contained within the borders.

@braddialpad braddialpad requested a review from iropolo July 10, 2024 20:57
@iropolo
Copy link
Contributor

iropolo commented Jul 10, 2024

On Conversation View -> Message input -> Default vue2 if you press enter, it doesn't add a jump line, it does it in vue3

It's actually only supposed to add a line if you use "shift + enter" pressing enter normally would send the message. I will look into this.

K, seems like in vue3 it sends the actions and also adds a jump line.

Comment on lines 72 to 73
onKeyDown ({ event }) {
if (this.items.length === 0) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like not working, I can still trigger errors in console pressing arrow keys

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like not working, I can still trigger errors in console pressing arrow keys

hmm can you tell me the steps you take to trigger this?

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh, seems like this.items is not empty on a second search so it always pass this validation.

Check the video, after I type @ I start to press the down arrow key.
Then I delete it and type again @ and start to press down arrow key, console error appears.

Screen.Recording.2024-07-10.at.18.55.06.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

There is another issue but it was there before.
If you press up arrow key it triggers a console error that seems to be related to destroy the popup (probably because you are moving out the @)

To reproduce type @ and quickly up arrow key.

Screen.Recording.2024-07-10.at.18.58.08.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check the video, after I type @ I start to press the down arrow key.
Then I delete it and type again @ and start to press down arrow key, console error appears.

Ah yeah, seems to happen if you press the arrow key before the dialog is loaded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variables for the component and the popup in suggestion.js were not getting set to null after being destroyed. They still had an object in them even though they were destroyed which kept many things "alive" when they shouldn't have been.

Should be fixed now.

@braddialpad braddialpad requested a review from iropolo July 10, 2024 23:36
@bianca-artola-dialpad
Copy link
Contributor

Sorry for disrupting here, but I noticed some issues with the input when adding :, @ or # and then enter

Screen.Recording.2024-07-11.at.09.16.01.mov

probably we need to execute submit action on those cases?

@braddialpad
Copy link
Contributor Author

Sorry for disrupting here, but I noticed some issues with the input when adding :, @ or # and then enter

Ah yeah, that's definitely a problem, thanks.

@braddialpad
Copy link
Contributor Author

Sorry for disrupting here, but I noticed some issues with the input when adding :, @ or # and then enter

probably we need to execute submit action on those cases?

@bianca-artola-dialpad should be fixed now.

@francisrupert
Copy link
Contributor

francisrupert commented Jul 12, 2024

UPDATE: oops, I guess I meant to address this to @braddialpad (never mind @bianca-artola-dialpad)

New tiny (I promise) improvement while you're in here:

Change...

.dt-message-input--focused {
  ...
}

...to...

.dt-message-input:focus-within {
  ...
}

This will remove the awkward blinking observed as dt-message-input--focused is toggled (view the attached screencast). This tiny change will ensure any amount of keyboard or mouse actions with dt-message-input retains the style (shadow and bold border).

Oh, and I assume remove associated tests and the current toggling of dt-message-input--focused, assuming it's not used for anything other than styling the container.

Thanks much! Let me know if you've any questions.

Screen.Recording.2024-07-11.at.9.52.00.PM.mov

@bianca-artola-dialpad
Copy link
Contributor

Sorry for disrupting here, but I noticed some issues with the input when adding :, @ or # and then enter
probably we need to execute submit action on those cases?

@bianca-artola-dialpad should be fixed now.

yeah, I cannot reproduce it anymore, thank you!

@braddialpad
Copy link
Contributor Author

K, seems like in vue3 it sends the actions and also adds a jump line.

Fixed now.

Copy link

✔️ Deploy previews ready!
😎 Dialtone preview: https://dialtone.dialpad.com/deploy-previews/pr-402/
😎 Dialtone-vue 2 preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-402/
😎 Dialtone-vue 3 the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-402/

@braddialpad braddialpad merged commit c30b3a7 into staging Jul 12, 2024
11 checks passed
@braddialpad braddialpad deleted the fix/message-input-various branch July 12, 2024 19:24
juliodialpad pushed a commit that referenced this pull request Jul 12, 2024
# [9.55.0](dialtone/v9.54.2...dialtone/v9.55.0) (2024-07-12)

### Bug Fixes

* NO-JIRA corrupted pnpm-lock file ([4f732c8](4f732c8))
* **Rich Text Editor:** NO-JIRA multiple improvements ([#402](#402)) ([c30b3a7](c30b3a7))

### Documentation

* DLT-1787 implement custom TOC ([#397](#397)) ([bf82624](bf82624))
* **Logo:** DLT-1866 modify dialtone documentation logo ([#403](#403)) ([8047550](8047550))

### Features

* **Design Tokens:** DLT-1870 additional expressive typography tokens ([#404](#404)) ([2bdc3a5](2bdc3a5))
* **Design Tokens:** DLT-1871 added gold brand color ([#405](#405)) ([ab0d032](ab0d032))
* **Multi Select:** DLT-1845 allow duplicated names on multiselect ([#394](#394)) ([ff8be5f](ff8be5f))
* **Scrollbar:** DLT-1473 add scrollbar directive ([#391](#391)) ([05af2ff](05af2ff))
juliodialpad pushed a commit that referenced this pull request Jul 12, 2024
# [2.143.0](dialtone-vue2/v2.142.2...dialtone-vue2/v2.143.0) (2024-07-12)

### Bug Fixes

* **Rich Text Editor:** NO-JIRA multiple improvements ([#402](#402)) ([c30b3a7](c30b3a7))

### Features

* **Multi Select:** DLT-1845 allow duplicated names on multiselect ([#394](#394)) ([ff8be5f](ff8be5f))
* **Scrollbar:** DLT-1473 add scrollbar directive ([#391](#391)) ([05af2ff](05af2ff))
juliodialpad pushed a commit that referenced this pull request Jul 12, 2024
# [3.136.0](dialtone-vue3/v3.135.2...dialtone-vue3/v3.136.0) (2024-07-12)

### Bug Fixes

* **Rich Text Editor:** NO-JIRA multiple improvements ([#402](#402)) ([c30b3a7](c30b3a7))

### Features

* **Multi Select:** DLT-1845 allow duplicated names on multiselect ([#394](#394)) ([ff8be5f](ff8be5f))
* **Scrollbar:** DLT-1473 add scrollbar directive ([#391](#391)) ([05af2ff](05af2ff))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants