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

Hackathon rich text formatting #593

Draft
wants to merge 25 commits into
base: staging
Choose a base branch
from

Conversation

braddialpad
Copy link
Contributor

No description provided.

@braddialpad braddialpad self-assigned this Dec 13, 2024
Copy link

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 added the no-visual-test Add this tag when the PR does not need visual testing label Dec 13, 2024
Copy link

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

Comment on lines +45 to +61
.d-rich-text-editor-link-bubble-menu {
display: flex;
font: var(--dt-typography-body-md);
background-color: var(--dt-color-surface-primary);
border: 1px solid var(--dt-color-border-default);
border-radius: 2.8rem;

// Left-Border on all buttons except the first one (separators)
button + button {
border-left: 1px solid var(--dt-color-border-default);
}

button {
padding: var(--dt-space-300) var(--dt-space-450);
}
}

Copy link
Contributor

@francisrupert francisrupert Dec 19, 2024

Choose a reason for hiding this comment

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

I provided feedback with @francotirabasso on this and some other things, but the key thing:

I say this should be a Popover (or at least emulate Popover style) and just use three straight clear buttons.

Though of course confirm with Franco before doing so.

The result would be effectively...

<dt-popover class="d-p4">
  <dt-stack direction="row" gap="200">
    <dt-button kind="muted" importance="clear">Edit</dt-button>
    <dt-button kind="muted" importance="clear">Open link</dt-button>
    <dt-button kind="danger" importance="clear">Remove</dt-button>
  </dt-stack>
</dt-popover>
Screen.Recording.2024-12-19.at.4.47.52.PM.mov

Comment on lines +8 to +11
padding: var(--dt-space-200) var(--dt-space-300);
color: var(--dt-color-purple-400);
background-color: var(--dt-color-purple-100);
border-radius: var(--dt-size-200);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
padding: var(--dt-space-200) var(--dt-space-300);
color: var(--dt-color-purple-400);
background-color: var(--dt-color-purple-100);
border-radius: var(--dt-size-200);
padding: var(--dt-space-200);
color: var(--dt-color-foreground-warning);
background-color: var(--dt-color-surface-secondary-opaque);
border-radius: var(--dt-size-radius-200);
border: var(--dt-size-border-100) solid var(--dt-color-border-subtle);
font: var(--dt-typography-code-md);
image

@francisrupert
Copy link
Contributor

For .d-rich-text-editor__code-block (not the inline one) add:

font: var(--dt-typography-code-md);

(I'm too lazy to find it in the file changes diff)

image

@francisrupert
Copy link
Contributor

Line break issue when when converting multiple lines of text to codeblock

Screen.Recording.2024-12-19.at.4.19.13.PM.mov

@francisrupert
Copy link
Contributor

When there are multiple links, it's difficult to move text cursor around.

Screen.Recording.2024-12-19.at.4.29.34.PM.mov

@francisrupert
Copy link
Contributor

  • Buttons should only be functional if focus is in compose field.
  • No button should be shown as active when focus is not in compose field.
Screen.Recording.2024-12-19.at.4.51.01.PM.mov

@francisrupert
Copy link
Contributor

A nested list of a different type should only reflect that type as active. Currently it shows as being both number and bullet. Technically true, though ideally the active one should only reflect that type even if nested.

Screen.Recording.2024-12-19.at.4.56.32.PM.mov

@@ -30,11 +37,28 @@
blockquote {
margin-left: 0;
padding-left: var(--dt-space-400);
border-left: var(--dt-size-border-300) solid var(--dt-color-foreground-muted-inverted);
border-left: var(--dt-size-border-300) solid var(--dt-color-foreground-secondary-inverted);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
border-left: var(--dt-size-border-300) solid var(--dt-color-foreground-secondary-inverted);
border-left: var(--dt-size-border-300) solid var(--dt-color-border-subtle);

@francisrupert
Copy link
Contributor

francisrupert commented Dec 19, 2024

Each action definitely needs a Tooltip, including keyboard shortcut if it has one.

@francotirabasso, which treatment do you like? The first one uses Keyboard Shortcut component. The second one is just tertiary color text.

image

@francisrupert
Copy link
Contributor

@braddialpad, @francotirabasso,

I wonder if we should provide keyboard accessibility to the link options when the cursor is within the link. Gmail example attached:

Screen.Recording.2024-12-20.at.11.40.22.AM.mov

@braddialpad
Copy link
Contributor Author

Thanks for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-visual-test Add this tag when the PR does not need visual testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants