refactor: clean ui modal component (#39654)

1. use "button" element for the modal close button
2. remove "ui modal" width hacks
This commit is contained in:
wxiaoguang authored and GitHub committed 2026-10-07 11:49:50 -07:00
1 parent 5bcb0ce0fc
commit 21cf3f774d
6 files changed
+36 -199

No files matched your search

+1 -1
View File
@@ -64,7 +64,7 @@
</div>
<div class="ui g-modal-confirm modal" id="test-modal-danger">
<div class="header">dangerous action dialog {{svg "octicon-x" 16 "close-modal"}}</div>
<div class="header">dangerous action dialog <button type="button" class="close-modal"></button></div>
<div class="content">hello, this is the modal dialog content, this is a dangerous operation</div>
{{template "base/modal_actions_confirm" (dict "ModalButtonDangerText" "I know and must do this is dangerous operation")}}
</div>
+30 -186
View File
@@ -1,5 +1,5 @@
/* These are the remnants of the fomantic modal module */
/* to keep the changes minimal and easy to revert or fine tune, the legacy classes
like "small" and "mini" are still kept for the HTML elements, these can be cleaned up in the future */
.ui.modal {
position: absolute;
display: none;
@@ -9,13 +9,20 @@
border: none;
box-shadow: 1px 3px 3px 0 var(--color-shadow), 1px 3px 15px 2px var(--color-shadow);
transform-origin: 50% 25%;
flex: 0 0 auto;
border-radius: var(--border-radius);
user-select: text;
will-change: top, left, margin, transform, opacity;
width: fit-content;
/* on small screens, the modal should not exceed 90% of the viewport width;
on large screens, the modal should not exceed 800px in width (ref: ".ui.container" width is 1280px) */
max-width: min(800px, 90vw);
/* on small screens, the modal should not be smaller than 300px or 90vw in width;
on large screens, the minimum width is 30% of the viewport width, capped at 800px */
min-width: min(800px, max(min(300px, 90vw), 30vw));
}
.ui.modal > :first-child:not(.icon) {
.ui.modal > :first-child {
border-top-left-radius: var(--border-radius);
border-top-right-radius: var(--border-radius);
}
@@ -25,9 +32,6 @@
border-bottom-right-radius: var(--border-radius);
}
/* TODO: this "close" button was inherited and modified from Fomantic UI, actually it is not good enough:
* * the layout depends on absolute position, which is not good for responsive design
* * it is not good for accessibility, ideally it should use a "button" element */
.ui.modal .close-modal {
cursor: pointer;
position: absolute;
@@ -35,13 +39,24 @@
transform: translateY(-50%);
right: 1rem;
z-index: 1;
opacity: 0.8;
font-size: 1.25em;
color: var(--color-text);
opacity: 0.5;
padding: 0;
border: none;
background: none;
width: 1.5rem;
height: 1.5rem;
}
.ui.modal .close-modal::before {
content: "";
display: block;
width: 100%;
height: 100%;
background-color: currentcolor;
mask-image: var(--octicon-x);
mask-size: cover;
}
.ui.modal .close-modal:hover {
opacity: 1;
}
@@ -49,10 +64,11 @@
.ui.modal > .header {
display: block;
position: relative;
font-family: var(--fonts-regular);
font-size: 18px;
font-weight: var(--font-weight-medium);
background: var(--color-body);
margin: 0;
padding: 1.25rem 1.5rem;
padding: 1.25rem 3rem 1.25rem 1.5rem; /* leave more right space for the "close" button */
box-shadow: none;
color: var(--color-text-dark);
border-bottom: 1px solid var(--color-secondary);
@@ -60,12 +76,6 @@
border-top-right-radius: var(--border-radius);
}
.ui.modal > .header:not(.ui) {
font-size: 1.42857143rem;
line-height: 1.28571429em;
font-weight: var(--font-weight-medium);
}
.ui.modal > .content,
.ui.modal form > .content {
display: block;
@@ -87,174 +97,10 @@
border-radius: 0 0 var(--border-radius) var(--border-radius);
}
.ui.modal .actions > .button {
margin-left: 0.75em;
}
@media only screen and (max-width: 767.98px) {
.ui.modal {
width: 95%;
}
}
@media only screen and (min-width: 768px) {
.ui.modal {
width: 88%;
}
}
@media only screen and (min-width: 992px) {
.ui.modal {
width: 850px;
}
}
@media only screen and (min-width: 1200px) {
.ui.modal {
width: 900px;
}
}
@media only screen and (min-width: 1920px) {
.ui.modal {
width: 950px;
}
}
@media only screen and (max-width: 991.98px) {
.ui.modal > .header {
padding-right: 2.25rem;
}
}
@media only screen and (max-width: 767.98px) {
.ui.modal > .header {
padding: 0.75rem 1rem !important;
padding-right: 2.25rem !important;
}
.ui.modal > .content {
display: block;
padding: 1rem !important;
}
.ui.modal > .actions {
padding: 1rem 1rem 0 !important;
}
.ui.modal .actions > .button {
margin-bottom: 1rem;
}
}
.ui.active.modal {
display: block;
}
.ui.mini.modal > .header:not(.ui) {
font-size: 1.3em;
}
@media only screen and (max-width: 767.98px) {
.ui.mini.modal {
width: 95%;
}
}
@media only screen and (min-width: 768px) {
.ui.mini.modal {
width: 35.2%;
}
}
@media only screen and (min-width: 992px) {
.ui.mini.modal {
width: 340px;
}
}
@media only screen and (min-width: 1200px) {
.ui.mini.modal {
width: 360px;
}
}
@media only screen and (min-width: 1920px) {
.ui.mini.modal {
width: 380px;
}
}
.ui.tiny.modal > .header:not(.ui) {
font-size: 1.3em;
}
@media only screen and (max-width: 767.98px) {
.ui.tiny.modal {
width: 95%;
}
}
@media only screen and (min-width: 768px) {
.ui.tiny.modal {
width: 52.8%;
}
}
@media only screen and (min-width: 992px) {
.ui.tiny.modal {
width: 510px;
}
}
@media only screen and (min-width: 1200px) {
.ui.tiny.modal {
width: 540px;
}
}
@media only screen and (min-width: 1920px) {
.ui.tiny.modal {
width: 570px;
}
}
.ui.small.modal > .header:not(.ui) {
font-size: 1.3em;
}
@media only screen and (max-width: 767.98px) {
.ui.small.modal {
width: 95%;
}
}
@media only screen and (min-width: 768px) {
.ui.small.modal {
width: 70.4%;
}
}
@media only screen and (min-width: 992px) {
.ui.small.modal {
width: 680px;
}
}
@media only screen and (min-width: 1200px) {
.ui.small.modal {
width: 720px;
}
}
@media only screen and (min-width: 1920px) {
.ui.small.modal {
width: 760px;
}
}
.ui.modal.g-modal-confirm {
max-width: min(800px, 90vw);
width: fit-content;
}
.ui.modal .content > form > .actions,
.ui.modal .content > .actions {
padding-top: 1em;
@@ -263,8 +109,10 @@
.ui.modal .actions > .ui.button {
display: inline-flex;
gap: var(--gap-inline);
align-items: center;
padding: 10px 12px 10px 10px;
margin-left: 0.75em;
margin-right: 0;
}
@@ -274,7 +122,3 @@
margin: 0 auto;
text-align: center;
}
.ui.modal .actions > .ui.button .svg {
margin-right: 5px;
}
+2 -2
View File
@@ -18,14 +18,14 @@ function showContentHistoryDetail(issueBaseUrl: string, commentId: string, histo
<div class="ui modal content-history-detail-dialog">
<div class="header flex-left-right">
<div>${htmlRaw(itemTitleHtml)}</div>
<div class="ui dropdown dialog-header-options tw-mr-8 tw-hidden">
<div class="ui dropdown dialog-header-options tw-mx-4 tw-hidden">
${i18nTextOptions}
${svgRaw('octicon-triangle-down', 14, 'dropdown icon')}
<div class="menu">
<div class="item tw-text-red" data-option-item="delete">${i18nTextDeleteFromHistory}</div>
</div>
</div>
${svgRaw('octicon-x', 16, 'close-modal')}
<button type="button" class="close-modal"></button>
</div>
<div class="comment-diff-data is-loading"></div>
</div>
+2
View File
@@ -40,10 +40,12 @@ function ariaModalFn(this: JQueryElem, ...args: Parameters<FomanticInitFunction>
const ret = fomanticModalFn.apply(this, args);
if (args[0] === 'show' || args[0]?.autoShow) {
for (const el of this) {
el.querySelector('.close-modal')?.setAttribute('aria-label', 'Close');
// If there is a form in the modal, there might be a "cancel" button before "ok" button (all buttons are "type=submit" by default).
// In such case, the "Enter" key will trigger the "cancel" button instead of "ok" button, then the dialog will be closed.
// It breaks the user experience - the "Enter" key should confirm the dialog and submit the form.
// So, all "cancel" buttons without "[type]" must be marked as "type=button".
// Although there are lint rules for "button type", some HTML code is generated in JS/Vue/Go which isn't linted, so we still need this patch.
for (const button of el.querySelectorAll('form button.cancel:not([type])')) {
button.setAttribute('type', 'button');
}
+1 -2
View File
@@ -2,7 +2,6 @@ import {registerGlobalInitFunc} from './observer.ts';
import {createElementFromHTML, hideElem, toggleElem} from '../utils/dom.ts';
import {showFomanticModal} from './fomantic/modal.ts';
import {html} from '../utils/html.ts';
import {svgRaw} from '../svg.ts';
type ShortcutHandler = (el: HTMLElement) => boolean;
@@ -76,7 +75,7 @@ function showShortcutHelp() {
<div id="global-shortcut-help" class="ui small modal" role="dialog" aria-modal="true">
<div class="header">
Keyboard Shortcuts
${svgRaw('octicon-x', 16, 'close-modal')}
<button type="button" class="close-modal"></button>
</div>
<div class="scrolling content">
<table class="ui very basic compact unstackable table">
-8
View File
@@ -82,14 +82,6 @@ export function queryElems<T extends HTMLElement>(parent: Element | ParentNode,
return applyElemsCallback<T>(parent.querySelectorAll(selector), fn);
}
export function onDomReady(cb: () => Promisable<void>) {
if (document.readyState === 'loading') {
document.addEventListener('DOMContentLoaded', cb);
} else {
cb();
}
}
/** checks whether an element is owned by the current document, and whether it is a document fragment or element node
* if it is, it means it is a "normal" element managed by us, which can be modified safely. */
export function isDocumentFragmentOrElementNode(el: Node) {