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

Jump to bottom

refactor: refactor overlay styling to use css class #4861

Closed
wants to merge 3 commits into from
Closed

refactor: refactor overlay styling to use css class #4861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
70aee85
refactor: refactor overlay styling to use css class
malcolm-kee May 13, 2023
a4d3b07
fix: small DRY cleanup
malcolm-kee May 13, 2023
415692a
fix: small CSS typo
malcolm-kee May 13, 2023
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
39 changes: 23 additions & 16 deletions client-src/overlay.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
import createOverlayMachine from "./overlay/state-machine.js";
import {
containerStyle,
createCssLoader,
dismissButtonStyle,
headerStyle,
iframeStyle,
Expand Down Expand Up @@ -93,6 +94,8 @@ const createOverlay = (options) => {
let containerElement;
/** @type {HTMLDivElement | null | undefined} */
let headerElement;
/** @type {import('./overlay/styles.js').CssLoader} */
let iframeCssLoader;
/** @type {Array<(element: HTMLDivElement) => void>} */
let onLoadQueue = [];
/** @type {TrustedTypePolicy | undefined} */
Expand All @@ -109,6 +112,16 @@ const createOverlay = (options) => {
});
}

/**
*
* @param {HTMLElement} element
* @param {{className: string, css: string}} styles
*/
function applyCss(element, styles) {
element.className = styles.className;
iframeCssLoader.load(styles.css);
}

/**
* @param {string | null} trustedTypesPolicyName
*/
Expand All @@ -129,12 +142,10 @@ const createOverlay = (options) => {
applyStyle(iframeContainerElement, iframeStyle);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intentionally keep iframe styling to use inline style because we can't scope CSS for it.


iframeContainerElement.onload = () => {
const contentElement =
/** @type {Document} */
(
/** @type {HTMLIFrameElement} */
(iframeContainerElement).contentDocument
).createElement("div");
const iframeDoc = iframeContainerElement.contentDocument;
iframeCssLoader = createCssLoader(iframeDoc);

const contentElement = iframeDoc.createElement("div");
containerElement =
/** @type {Document} */
(
Expand All @@ -143,16 +154,16 @@ const createOverlay = (options) => {
).createElement("div");

contentElement.id = "webpack-dev-server-client-overlay-div";
applyStyle(contentElement, containerStyle);
applyCss(contentElement, containerStyle);

headerElement = document.createElement("div");

headerElement.innerText = "Compiled with problems:";
applyStyle(headerElement, headerStyle);
applyCss(headerElement, headerStyle);

const closeButtonElement = document.createElement("button");

applyStyle(closeButtonElement, dismissButtonStyle);
applyCss(closeButtonElement, dismissButtonStyle);

closeButtonElement.innerText = "×";
closeButtonElement.ariaLabel = "Dismiss";
Expand Down Expand Up @@ -236,19 +247,15 @@ const createOverlay = (options) => {
const entryElement = document.createElement("div");
const msgStyle =
type === "warning" ? msgStyles.warning : msgStyles.error;
applyStyle(entryElement, {
...msgStyle,
padding: "1rem 1rem 1.5rem 1rem",
});
applyCss(entryElement, msgStyle);

const typeElement = document.createElement("div");
const { header, body } = formatProblem(type, message);

typeElement.innerText = header;
applyStyle(typeElement, msgTypeStyle);
applyCss(typeElement, msgTypeStyle);

if (message.moduleIdentifier) {
applyStyle(typeElement, { cursor: "pointer" });
// element.dataset not supported in IE
typeElement.setAttribute("data-can-open", true);
typeElement.addEventListener("click", () => {
Expand All @@ -261,7 +268,7 @@ const createOverlay = (options) => {
// Make it look similar to our terminal.
const text = ansiHTML(encode(body));
const messageTextNode = document.createElement("div");
applyStyle(messageTextNode, msgTextStyle);
applyCss(messageTextNode, msgTextStyle);

messageTextNode.innerHTML = overlayTrustedTypesPolicy
? overlayTrustedTypesPolicy.createHTML(text)
Expand Down
166 changes: 116 additions & 50 deletions client-src/overlay/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,7 @@
// styles are inspired by `react-error-overlay`

const msgStyles = {
error: {
backgroundColor: "rgba(206, 17, 38, 0.1)",
color: "#fccfcf",
},
warning: {
backgroundColor: "rgba(251, 245, 180, 0.1)",
color: "#fbf5b4",
},
};
// The class names are quite generic, but it should be fine since they are
// scoped to the iframe only.

const iframeStyle = {
position: "fixed",
Expand All @@ -24,58 +16,131 @@ const iframeStyle = {
};

const containerStyle = {
position: "fixed",
boxSizing: "border-box",
left: 0,
top: 0,
right: 0,
bottom: 0,
width: "100vw",
height: "100vh",
fontSize: "large",
padding: "2rem 2rem 4rem 2rem",
lineHeight: "1.2",
whiteSpace: "pre-wrap",
overflow: "auto",
backgroundColor: "rgba(0, 0, 0, 0.9)",
color: "white",
className: "webpack-container",
css: /* css */ `.webpack-container {
position: fixed;
box-sizing: border-box;
left: 0;
top: 0;
right: 0;
bottom: 0;
width: 100vw;
height: 100vh;
font-size: large;
padding: 2rem 2rem 4rem 2rem;
line-height: 1.2;
white-space: pre-wrap;
overflow: auto;
background-color: rgba(0, 0, 0, 0.9);
color: white;
}
`,
};

const headerStyle = {
color: "#e83b46",
fontSize: "2em",
whiteSpace: "pre-wrap",
fontFamily: "sans-serif",
margin: "0 2rem 2rem 0",
flex: "0 0 auto",
maxHeight: "50%",
overflow: "auto",
className: "webpack-header",
css: /* css */ `.webpack-header {
color: #e83b46;
font-size: 2em;
font-family: sans-serif;
white-space: pre-wrap;
margin: 0 2rem 2rem 0;
flex: 0 0 auto;
max-height: 50%;
overflow: auto;
}
`,
};

const dismissButtonStyle = {
color: "#ffffff",
lineHeight: "1rem",
fontSize: "1.5rem",
padding: "1rem",
cursor: "pointer",
position: "absolute",
right: 0,
top: 0,
backgroundColor: "transparent",
border: "none",
className: "webpack-dismiss-btn",
css: /* css */ `.webpack-dismiss-btn {
color: #ffffff;
line-height: 1rem;
font-size: 1.5rem;
padding: 1rem;
cursor: pointer;
position: absolute;
right: 0;
top: 0;
background-color: transparent;
border: none;
}

.webpack-dismiss-btn:hover {
color: #d1d5db;
}`,
};

const msgTypeStyle = {
color: "#e83b46",
fontSize: "1.2em",
marginBottom: "1rem",
fontFamily: "sans-serif",
className: "webpack-msg-type",
css: /* css */ `.webpack-msg-type {
margin-bottom: 1rem;
color: #e83b46;
font-size: 1.2em;
font-family: sans-serif;
}

.webpack-msg-type[data-can-open] {
cursor: pointer;
}
`,
};

const msgTextStyle = {
lineHeight: "1.5",
fontSize: "1rem",
fontFamily: "Menlo, Consolas, monospace",
className: "webpack-msg-text",
css: /* css */ `.webpack-msg-text {
line-height: 1.5;
font-size: 1rem;
font-family: ui-monospace, SFMono-Regular, Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lovingly copied from TailwindCSS font-mono: https://tailwindcss.com/docs/font-family

}
`,
};

const msgStyles = {
error: {
className: "webpack-error-msg",
css: /* css */ `.webpack-error-msg {
background-color: rgba(206, 17, 38, 0.1);
color: #fccfcf;
padding: 1rem 1rem 1.5rem 1rem;
}`,
},
warning: {
className: "webpack-warning-msg",
css: /* css */ `.webpack-warning-msg {
background-color: rgba(251, 245, 180, 0.1);
color: #fbf5b4;
padding: 1rem 1rem 1.5rem 1rem;
}`,
},
};

/**
* @typedef {Object} CssLoader
* @property {(css: string) => void} load
*/

/**
*
* @param {Document} doc
* @return {CssLoader}
*/
const createCssLoader = (doc) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

css loader could be confusing? Any suggestion is welcome.

/** @type {string[]} */
const loadedCss = [];

return {
load: (css) => {
// ignore CSS rule that has loaded before
if (!loadedCss.includes(css)) {
const style = doc.createElement("style");
style.innerHTML = css;
doc.head.appendChild(style);
loadedCss.push(css);
}
},
};
Copy link
Member

Choose a reason for hiding this comment

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

It can create a lot of problems. Shorty - we already tried this solution and we had a lot of issues about style tags and its order and overlapping and etc and a lot, so I prefer do not use style and classes at all, I am fine with refactor and move styles in own objects with good names, but using classes again make more problems than benefits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Any issues that was raised before on this? Maybe worth include them in the comments

Copy link
Member

Choose a reason for hiding this comment

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

They were, but they are difficult to find because they are scattered and many headings do not match the original problem, I tried to search for them, but unfortunately I can not find but I remember very well that we had problems with this 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, tag me when you stumble them then.

};

export {
Expand All @@ -86,4 +151,5 @@ export {
dismissButtonStyle,
msgTypeStyle,
msgTextStyle,
createCssLoader,
};
Loading