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

Show something when embedding something non-previewable #8435

Closed
Reinmar opened this issue Nov 9, 2020 · 3 comments Β· Fixed by #8799
Closed

Show something when embedding something non-previewable #8435

Reinmar opened this issue Nov 9, 2020 · 3 comments Β· Fixed by #8799
Assignees
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:html-embed type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@Reinmar
Copy link
Member

Reinmar commented Nov 9, 2020

πŸ“ Provide a description of the improvement

It currently looks like this:

The problem is not that it's not clear that this is an HTML snippet, but rather – what snippet this is. If you have a couple of them in the content, you may not understand which is which or if they are filled already or not.

Therefore, some way to identify what's inside would be good.

Option 1. Show non-editable source

We could simply show a disabled textarea with the source of the embed, just like in the case of when previews are completely disabled.

However, how can we detect that a preview is not available?

One option is, of course, that when the sanitizer stripped everything, we know that the thing is non-previewable. But what if just one part of the input was removed and it makes the preview non-existent?

The other option is to make this check after rendering the preview. If it is 0px high, we can tell that the preview didn't render. However, this will be slow and may cause layout trashing so if we go this way we must be smart.

Related ticket (in this option): #8326.

Option 2. Add a "title" field

The other option would be to add a field for a snippet's title and display it e.g. in the "HTML snippet: <title>" place.

The title could be stored in data-title and we wouldn't have to do anything more – if the preview doesn't render, the user still sees something. We don't have to even detect these situations.

My biggest concern is that the title will leak to data, but I'd be fine with that as it's kind of a "label/caption" like thing.


If you'd like to see this improvement implemented, add a πŸ‘ reaction to this post.

@Reinmar Reinmar added type:improvement This issue reports a possible enhancement of an existing feature. package:html-embed squad:core Issue to be handled by the Core team. labels Nov 9, 2020
@Reinmar
Copy link
Member Author

Reinmar commented Nov 9, 2020

Option 3. Always show the banner from #8434.

Pros: Just like that.

Cons: Will be displayed for every embed and pollute the screen.

Option 4: Show the banner from #8434 when we detected a change by sanitizer.

Cons: Would require #8326. And still may not work.

@Reinmar Reinmar added this to the nice-to-have milestone Nov 9, 2020
@Reinmar Reinmar added domain:ui/ux This issue reports a problem related to UI or UX. and removed squad:core Issue to be handled by the Core team. labels Nov 9, 2020
@oleq oleq modified the milestones: nice-to-have, backlog, iteration 39 Jan 4, 2021
@magda-chrzescian
Copy link
Contributor

I introduced a solution in the PR.

I added a placeholder in the preview container. If any preview is visible, the placeholder is simply getting covered by it.

@magda-chrzescian
Copy link
Contributor

One more follow-up: #8836

oleq added a commit that referenced this issue Jan 18, 2021
Other (html-embed): A placeholder should be displayed if the HTML snippet is not previewable or empty. Closes #8435.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:html-embed type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants