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

New feature: Callouts #2227

Merged
merged 20 commits into from
Mar 15, 2022
Merged

New feature: Callouts #2227

merged 20 commits into from
Mar 15, 2022

Conversation

vinicius73
Copy link
Member

@vinicius73 vinicius73 commented Mar 7, 2022

Summary

Add a new feature called Callouts

image

image

It allows putting more than one paragraph.
image

@vinicius73 vinicius73 requested a review from max-nextcloud March 7, 2022 19:44
@vinicius73
Copy link
Member Author

We also have tried some variations of background color.

image

And icon colors.

image image

The main problem of the alternatives is the text and background combination, to create a good contrast.
Despite using CSS variables like --color-primary-light, --color-warning-hover, --color-error-hover and --color-success-hover to make easy to user change the color of blocks. It's hard to define a good solution for background and text color contrast.

Those variables are in hexadecimal, it should be rgba or hsla to be possible to define a good and automatic color change for readability.

Refs: https://css-tricks.com/switch-font-color-for-different-backgrounds-with-css/

@juliusknorr juliusknorr added 2. developing enhancement New feature or request labels Mar 7, 2022
@juliusknorr juliusknorr added this to the Nextcloud 24 milestone Mar 7, 2022
@jancborchardt
Copy link
Member

jancborchardt commented Mar 7, 2022

Very nice! :) Some design feedback:

  • A more descriptive and shorter word for this would be "Callouts".
  • The warning icon would look better as a triangle: https://fonts.google.com/icons?selected=Material%20Icons%3Awarning%3A
  • Would be better with some border-radius just like the "Code block" element: var(--border-radius)
  • Good on trying out the different backgrounds – what would be best is indeed using a -light variant of each of the colors also in the background. For primary, we use 10% saturation of the color for that. But it’s also fine as is now and can be done separately, since you say it needs the variables to be adjusted.

@max-nextcloud max-nextcloud requested review from jancborchardt and removed request for Dagefoerde March 8, 2022 06:07
@nimishavijay
Copy link
Member

Looks great! And agreed with Jan, left border for each type of callout could be 100% opacity of its corresponding color, and background could be 10% opacity of its corresponding color. If we are using --color-main-text for the text then it should have enough contrast. It would look like this:
image
Additional tiny bit of feedback from me, we could use --color-main-text for the icon colors also, seems like it is #000 now.

@julien-nc
Copy link
Member

This looks very nice!

I don't know about using the primary color for info blocks. It looks weird when it is similar to the warn/error/success color. How about something blueish or greyish?

Those variables are in hexadecimal, it should be rgba or hsla to be possible to define a good and automatic color change for readability.

While waiting for potential new css vars for the light backgrounds, we could just hardcode them:
rgba(236, 167, 0, .1) for warning for example.

I played with your changes, made some changes to img/warn.svg and it's not effective. How can we make sure new versions of those text-specific icons are loaded?

@max-nextcloud
Copy link
Collaborator

I played with your changes, made some changes to img/warn.svg and it's not effective. How can we make sure new versions of those text-specific icons are loaded?

I think i had similar issues in the past. I think the server is caching the images. Setting up my docker dev environment from scratch fixed it for me - but that is obviously far from ideal. I suspect there is some occ command or so to clean up image cache or even disable it.

},
},
{
label: t('text', 'Error'),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
label: t('text', 'Error'),
label: t('text', 'Danger'),

Maybe danger would be a bit less technical as the user facing term?


name: 'calloutContainer',

content: 'paragraph+',
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, is limiting to paragraph done on purpose or would there be also a way to have other blocks like headings/lists in the callout container as well? Fine to keep it but if there are limitations maybe a quick code comment might be good about the reasoning ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@vinicius73 and me came up with this during a call.
One of the issues we were facing was having different callouts nested - like a warning inside an info. That would be super confusing.

I also thought it would be good to try and keep the schema as simple as possible to make it easier to reason about. I was afraid we might create such complex structures in text that the generated markdown will not be parsed by other markdown consumers. Today i tried to create such a document by nesting lists and quotes etc. and the resulting markdown was parsed well by github. So markdown is able to handle much more complexity than i thought.

So now i think we actually need to allow for the same level of complexity in the schema. If we have a file that has content that the schema does not allow for it will fail to load in text. In case of the callouts this would currently already be invalid:

::: info
### heading in the info
Some text
:::

So I think we should allow for all block content in infoboxes to be able to parse markdown that contains these.

At the same time I would like to try to discourage creating too complex structures in the UI. For example... when you are in an info box and click the warning box button... we turn the infobox into a warning box rather than adding a warning box inside the info box.

Copy link
Member

Choose a reason for hiding this comment

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

I'd be fine either way, but iirc yesterday (in our call after the message) you mentioned that now you think that supporting different node types would be more fitting to what markdown can actually represent.

We could also get this in first with that limitation and look into the further sub-node types in a follow up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When working on tables i noticed that content that is out of spec for prosemirror will still be inserted as the text content.
So this:

::: info

## Heading

content
:::

Would render like

::: info

Heading

content
:::

Which seems fine for a start. I would also be okay with allowing headings in there - but let's avoid any generic container such as lists or quotes that then allow everything inside them.

@juliusknorr
Copy link
Member

I think i had similar issues in the past. I think the server is caching the images. Setting up my docker dev environment from scratch fixed it for me - but that is obviously far from ideal. I suspect there is some occ command or so to clean up image cache or even disable it.

During development a occ maintenance:repair and redis-cli flushall should do the trick there.

@nimishavijay
Copy link
Member

I don't know about using the primary color for info blocks. It looks weird when it is similar to the warn/error/success color. How about something blueish or greyish?

So @jancborchardt and I talked about this, and we think we could use Nextcloud blue #0082c9 but we should make it a separate variable like --color-info or similar :)

@vinicius73 vinicius73 force-pushed the feature/2184-custom-containers branch from 7b9cc7b to 322645b Compare March 10, 2022 14:58
@juliusknorr juliusknorr self-requested a review March 10, 2022 15:27
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Looks good as discussed with removal of the line height adjustments 👍

Lets give @max-nextcloud the chance for a review and feedback on the content nodes before merging 😉

@vinicius73 vinicius73 changed the title #2184 Custom containers New feature: Callouts Mar 10, 2022
@vinicius73 vinicius73 marked this pull request as ready for review March 10, 2022 15:56
Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Looks great - both code and ui wise. 🤩

One minor thing that i would like would be if removing multiline callouts worked like changing them to a different type. When i am on one line and i turn the callout off it would be nice if the entire callout would be gone - not just the line i am on. Could be done separately though...

I'll approve this so we can get it in.

Vinicius Reis added 5 commits March 14, 2022 11:38
using help-circle from material icons
we expect to use i icon for the custom-containers feature

Signed-off-by: Vinicius Reis <[email protected]>
create a tiptap extention to create a custom-container element, missing markdown-it-container

Signed-off-by: Vinicius Reis <[email protected]>
define info, warn, error and success containers

Signed-off-by: Vinicius Reis <[email protected]>
Signed-off-by: Vinicius Reis <[email protected]>
use type in toMarkdown

Signed-off-by: Vinicius Reis <[email protected]>
Vinicius Reis added 14 commits March 14, 2022 11:38
also add some css improviments

Signed-off-by: Vinicius Reis <[email protected]>
All content was transformed into plain text, missing all node data.

Signed-off-by: Vinicius Reis <[email protected]>
- Only paragraph inside container
- Change and remove container

Signed-off-by: Vinicius Reis <[email protected]>
Also add container icons

Signed-off-by: Vinicius Reis <[email protected]>
Signed-off-by: Vinicius Reis <[email protected]>
remove unecessary width

Signed-off-by: Vinicius Reis <[email protected]>
also remove dependency vulnerability

Signed-off-by: Vinicius Reis <[email protected]>
Signed-off-by: Vinicius Reis <[email protected]>
Signed-off-by: Vinicius Reis <[email protected]>
@vinicius73 vinicius73 force-pushed the feature/2184-custom-containers branch from 944b062 to 2874299 Compare March 14, 2022 14:42
@juliusknorr juliusknorr mentioned this pull request Mar 14, 2022
39 tasks
Signed-off-by: Vinicius Reis <[email protected]>
@vinicius73 vinicius73 force-pushed the feature/2184-custom-containers branch from d344ad6 to 1bcf179 Compare March 14, 2022 22:06
@juliusknorr juliusknorr merged commit 646376b into master Mar 15, 2022
@delete-merged-branch delete-merged-branch bot deleted the feature/2184-custom-containers branch March 15, 2022 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Callout box support
6 participants