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.

Sign up for GitHub

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

Flyout: define position="inline" #500

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Flyout: define position="inline" #500

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
186fde2
Flyout: define `position="inline"`
humitos Jan 16, 2025
9c8806d
Add some padding to the flyout respecting the container
humitos Jan 16, 2025
c242b50
Update tests to pass
humitos Jan 16, 2025
ad9fc4e
Use inline positioning instead of absolute (#501)
agjohnson Jan 20, 2025
f95fae5
I added my changes
humitos Jan 20, 2025
55ab856
Rename CSS classes
humitos Jan 20, 2025
60b9e33
Increase the size of the container and grow with it
humitos Jan 20, 2025
3372883
Fix down/up openings
humitos Jan 20, 2025
fa162e5
adjustments for Sphinx Furo theme
humitos Jan 20, 2025
721cfa7
Caculated properties are not needed
humitos Jan 21, 2025
bf6962c
Calculated width is not neeed here
humitos Jan 21, 2025
4af9427
Use JavaScript to calculate `margin-bottom` based on `main`'s height
humitos Jan 21, 2025
49e6b4e
Same for `.down` class
humitos Jan 21, 2025
8acadc3
Keep `width` because is required in some scenarios
humitos Jan 21, 2025
4e40ec2
Remove JS hack code
humitos Jan 23, 2025
bfde1ec
More CSS tricks
humitos Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions public/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,40 @@ <h2 id="ethicalads">Ethical Ad</h2>
<h2 id="customscript">CustomScript</h2>
<p>Content of the section</p>

<h2 id="flyout">Flyout</h2>

<p>
Lorem ipsum odor amet, consectetuer adipiscing elit. Ultricies massa
molestie per iaculis id cras fermentum mi fames. Feugiat per cubilia
volutpat; dictumst cras libero fringilla.
</p>

<div style="width: 20em; height: 2.5em; background: red;">
<readthedocs-flyout position="up inline"></readthedocs-flyout>
</div>

<p>
Placerat conubia vivamus finibus
vulputate, sit ipsum. Senectus netus aptent laoreet per venenatis maximus.
Praesent mi tellus congue orci mus in etiam suspendisse. Inceptos senectus
nam accumsan, praesent felis consequat feugiat maximus. Dolor cras
faucibus sociosqu feugiat urna orci integer hendrerit.
</p>

<!-- Pretend this is a sidebar menu -->
<div style="width: 35em; position: fixed; left: 0; top: 100; background: red;">
<readthedocs-flyout position="down inline"></readthedocs-flyout>
</div>

<!-- Default positioning -->
<readthedocs-flyout position="bottom-right"></readthedocs-flyout>

<p>
Felis mus quis urna consequat arcu blandit finibus dui. Rutrum fames
aliquet nunc vitae aenean class mattis elit. Quis pretium himenaeos
facilisis sodales molestie fringilla ac interdum.
</p>

<h2 id="search">Search</h2>
<p>Hit <code>/</code> to trigger the search modal, or click on the input from the flyout.</p>
<readthedocs-search></readthedocs-search>
Expand Down
45 changes: 40 additions & 5 deletions src/flyout.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,26 +20,38 @@
line-height: var(--addons-flyout-line-height);
}

.container.bottom-right {
.container.bottom-right:not(.inline) {
right: 20px;
bottom: 50px;
}

.container.bottom-left {
.container.bottom-left:not(.inline) {
left: 20px;
bottom: 50px;
}

.container.top-left {
.container.top-left:not(.inline) {
left: 20px;
top: 50px;
}

.container.top-right {
.container.top-right:not(.inline) {
right: 20px;
top: 50px;
}

.container.inline {
width: 100%;
max-width: 100%;
height: 100%;
max-height: 100%;
padding: 0 10px;
margin: 0;
position: relative;
display: inline-block;
overflow: visible;
}

:host > div {
font-family: var(
--readthedocs-flyout-font-family,
Expand Down Expand Up @@ -116,7 +128,30 @@ main {
margin-top: 5px;
}

main.closed {
.container.inline main {
position: absolute;
padding: 1em;
background-color: var(--readthedocs-flyout-background-color, rgb(39, 39, 37));

/* Remove the padding-left added in `.container.inline` */
margin-left: -10px;

/* Use full width minus 1em padding added on left and right in `.container.inline main` */
width: calc(100% - 2em);
}

.container.up.inline main {
top: unset;
bottom: 0;
}

.container.down.inline main {
top: 0;
bottom: unset;
}

main.closed,
.container.inline main.closed {
display: none;
}

Expand Down
27 changes: 24 additions & 3 deletions src/flyout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ export class FlyoutElement extends LitElement {
static properties = {
config: { state: true },
opened: { type: Boolean },
floating: { type: Boolean },
humitos marked this conversation as resolved.
Show resolved Hide resolved
position: { type: String },
};

Expand All @@ -37,7 +36,6 @@ export class FlyoutElement extends LitElement {

this.config = null;
this.opened = false;
this.floating = true;
this.position = "bottom-right";
this.readthedocsLogo = READTHEDOCS_LOGO;
}
Expand Down Expand Up @@ -327,10 +325,33 @@ export class FlyoutElement extends LitElement {
}

updateCSSClasses() {
this.classes = { floating: this.floating, container: true };
this.classes = { container: true };
this.classes[this.position] = true;
}

updated() {
// Update the `margin-bottom` property for the main part of the flyout,
// to display it exactly over the header of the flyout.
// I understand this is not possible to do with CSS, so we use JavaScript for it.
const main = this.renderRoot.querySelector("main");
const header = this.renderRoot.querySelector("header");
if (main !== null && header !== null) {
if (this.position.includes("up")) {
main.style.setProperty(
"margin-bottom",
`${header.getBoundingClientRect().height}px`,
);
}

if (this.position.includes("down")) {
main.style.setProperty(
"margin-top",
`${header.getBoundingClientRect().height}px`,
);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So we should always avoid doing anything with JS, especially with Element.style. What this property does is add inline style="" attribute to the element, which causes trouble with secure CSP rulesets and causes problems when users try to override our styles.

What value do we not know that we need to perform calculations in JS to get?

Copy link
Member Author

Choose a reason for hiding this comment

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

I know, but I did it in JS to show this is possible at least.

I wrote what I need to do in #500 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you know how to do that with pure CSS? 🤔

Copy link
Contributor

@agjohnson agjohnson Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In CSS this seems like it should just be addition. If we're mixing units, we should change them all to em so that the math is just a simple line-height + margin-bottom + margin-top + padding-bottom + padding-top. The header height should be static, we set the font size and don't allow wrapping, so there shouldn't be guessing there either.

What value do we not know that we need to perform calculations in JS to get?

This isn't clear to me still, so maybe I'm missing something here.

Copy link
Member Author

Choose a reason for hiding this comment

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

What value do we not know that we need to perform calculations in JS to get?

I need to calculate the main.margin-bottom's property based on the header.height's property. How I can do that in CSS?

header.height is not static since it will be based on the container where the theme author inject the <readthedocs-flyout> element. How I can get that value with pure CSS?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to calculate it because we should know this value.

header.height is not static since it will be based on the container where the theme author inject the element.

This doesn't seem correct. If the parent element is 10em, we get this effect if we try to fill vertical space with 100% height:

image

We instead want the header to be static height, and derived from font-size. We should be able to determine the value needed for margin-bottom just by math based on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I thought we wanted height and width to be dynamic and based on its container. If we want a fixed height, that should be easier to calculate, yeah.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. I added .container.closed { height: 2.5rem } and kept the .container { height: auto } so it grows accordingly when it's clicked.


render() {
// The element doesn't yet have our config, don't render it.
if (this.config === null) {
Expand Down
4 changes: 2 additions & 2 deletions tests/__snapshots__/flyout.test.snap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
export const snapshots = {};

snapshots["Flyout addon snapshot flyout completely"] =
`<div class="bottom-right container floating">
`<div class="bottom-right container">
<header>
<img
alt="Read the Docs"
Expand Down Expand Up @@ -121,7 +121,7 @@ snapshots["Flyout addon snapshot flyout completely"] =
/* end snapshot Flyout addon snapshot flyout completely */

snapshots["Flyout addon snapshot flyout with search disabled"] =
`<div class="bottom-right container floating">
`<div class="bottom-right container">
<header>
<img
alt="Read the Docs"
Expand Down