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

feat(dashboard): digest aggregated by key field editor with autosuggestions #7322

Merged
merged 7 commits into from
Dec 19, 2024

Conversation

LetItRock
Copy link
Contributor

What changed? Why was the change needed?

Digest - aggregated by key field support the editor with autosuggestions for the variables.

Screenshots

Screen.Recording.2024-12-18.at.12.15.04.mov

@LetItRock LetItRock self-assigned this Dec 18, 2024
Copy link

linear bot commented Dec 18, 2024

Copy link
Member

@BiswaViraj BiswaViraj left a comment

Choose a reason for hiding this comment

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

Nice 👌🏼

Not a blocker: We may need to add a custom scrollbar for the editor. the current one looks a bit clunky 😅

@scopsy
Copy link
Contributor

scopsy commented Dec 18, 2024

@LetItRock do we need to have the {{curly}} braces for it in the design? I was expecting not to have it and select the key itself. I'm concerned it's confusing and people won't understand the {{ are needed. cc. @twentyone24

className={cn(
'min-w-[20ch] [appearance:textfield] [&::-webkit-inner-spin-button]:appearance-none [&::-webkit-outer-spin-button]:appearance-none'
)}
<Editor
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that as part of future work, we need to introduce a single line editor component that leverages the <Editor/> but behaves like an input. That is, on Enter press no new limes should be added, and on tab press we should move to the next input in the form instead of applying indentation to the current line of text.

Base automatically changed from nv-4585-scheduled-digest to next December 19, 2024 10:15
@LetItRock
Copy link
Contributor Author

@LetItRock do we need to have the {{curly}} braces for it in the design? I was expecting not to have it and select the key itself. I'm concerned it's confusing and people won't understand the {{ are needed. cc. @twentyone24

@scopsy This is our approach in "almost" all editor fields; the field is free text plus variables. I know this one is a bit different, but I don't remember if we allow having multiple keys separated by a comma (like we had in v1)

Copy link

netlify bot commented Dec 19, 2024

Deploy Preview for dashboard-v2-novu-staging ready!

Name Link
🔨 Latest commit 6063b71
🔍 Latest deploy log https://app.netlify.com/sites/dashboard-v2-novu-staging/deploys/6763f61ba16092000877b8eb
😎 Deploy Preview https://deploy-preview-7322.dashboard-v2.novu-staging.co
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 19, 2024

Deploy Preview for dev-web-novu ready!

Name Link
🔨 Latest commit 6063b71
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/6763f61bb60a1800086971ea
😎 Deploy Preview https://deploy-preview-7322.dashboard.novu-staging.co
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@LetItRock LetItRock merged commit 42757a9 into next Dec 19, 2024
34 checks passed
@LetItRock LetItRock deleted the nv-5049-digest-aggregated-by-key-field-editor branch December 19, 2024 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants