-
Notifications
You must be signed in to change notification settings - Fork 3
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): DLT-2107 message input recipe updates #522
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.
Your linter is doing some magic here, please fix it and update the PR, it's pretty hard to review with all those automatic-linter changes.
You can try with steps 3 and 4 of this article: https://www.digitalocean.com/community/tutorials/linting-and-formatting-with-eslint-in-vs-code
Please add either the |
@juliodialpad I think I had some bad merges, so I reverted many of my changes to a solid point and re-implemented them. It looks like everything is building correctly now, so if you could review it, that would be appreciated! Thanks. |
@ninamarina Thanks for the catch. I mis-spelled "direction" (as As far as the opacity, it's something we can discuss. That is intentional (and is now reflected in Vue2 as well). The idea is that when the message input is in focus, we reduce the color strength of the buttons. I've reduced the opacity amount from 50% to 75% to make it a bit more subtle, but still cut the strength. |
{{ showAddLink.setLinkTitle }} | ||
</span> | ||
<dt-input | ||
v-model="linkInput" | ||
:input-aria-label="showAddLink.setLinkInputAriaLabel" | ||
data-qa="dt-editor-link-input" | ||
:placeholder="setLinkPlaceholder" | ||
input-wrapper-class="d-bgc-black-100 d-mt6 d-bar5 d-ba d-baw1 d-bc-black-300 d-py2 d-ol-none" | ||
input-wrapper-class="d-bgc-secondary d-mt6 d-bar5 d-ba d-baw1 d-bc-default d-py2 d-ol-none" |
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.
Are these changes in the Editor component intended?
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.
Yes. We shouldn't be using raw color tokens, especially when we have semantic surface and border color tokens that match appropriately. Can you confirm @francisrupert ?
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.
Of course, especially as they align to the Figma color use.
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.
Did realize though that the Vue sync scripts didn't copy over the updated classes from Vue3 to Vue2. Updated in Vue2 editor.vue now.
packages/dialtone-vue2/recipes/conversation_view/message_input/message_input.vue
Outdated
Show resolved
Hide resolved
packages/dialtone-vue2/recipes/conversation_view/message_input/message_input.vue
Outdated
Show resolved
Hide resolved
packages/dialtone-vue2/recipes/conversation_view/message_input/message_input.vue
Outdated
Show resolved
Hide resolved
@hynes-dialpad just left you some comments, let me know if you need help with any of those |
Thanks @ninamarina! I've made the necessary updates based on your feedback. |
It appears to be doing it on hover too, not just focus. Feels very odd, though I suppose it can still make sense since they're still actionable. Screen.Recording.2024-10-07.at.12.13.23.PM.mov |
@francisrupert Yeah, I added that after Nina's comment so that, on hover, the opacity reverts to normal levels so they don't look disabled. |
packages/dialtone-vue2/recipes/conversation_view/message_input/message_input.vue
Outdated
Show resolved
Hide resolved
packages/dialtone-vue2/recipes/conversation_view/message_input/message_input.vue
Outdated
Show resolved
Hide resolved
packages/dialtone-vue2/recipes/conversation_view/message_input/message_input.vue
Outdated
Show resolved
Hide resolved
packages/dialtone-vue2/recipes/conversation_view/message_input/message_input.vue
Show resolved
Hide resolved
packages/dialtone-vue2/recipes/conversation_view/message_input/message_input.vue
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.
Your linter is still doing a lot of changes on this file
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'm using VS Studio so not sure how.
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.
Approve from design perspective, including with Percy. Looks like the main followup is what Tico called out as a separate task to replace CSS Utilities with a custom style specific to this component to avoid CSS specificity battles when consuming and adjusting further.
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.
Looks good now, thanks! Just apply the suggested changes before merging.
packages/dialtone-vue2/recipes/conversation_view/message_input/message_input.vue
Outdated
Show resolved
Hide resolved
packages/dialtone-vue3/recipes/conversation_view/message_input/message_input.vue
Outdated
Show resolved
Hide resolved
βοΈ Deploy previews ready! |
Message Input Recipe Updates
Obligatory GIF (super important!)
π οΈ Type Of Change
These types will increment the version number on release:
π Jira Ticket
DLT-2107
π Description
Jira ticket outlines all changes in PR.
π Checklist
For all PRs:
For all Vue changes:
./scripts/dialtone-vue-sync.sh
script. Read docs here: Dialtone Vue Sync ScriptFor all CSS changes: