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

CLDR-17462 update ST status icons for clarity #3763

Merged
merged 7 commits into from
May 30, 2024
Merged

CLDR-17462 update ST status icons for clarity #3763

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
154c842
CLDR-17462 update ST status icons for clarity
srl295 May 29, 2024
4a10a6c
CLDR-17462 update ST status icons for clarity
srl295 May 30, 2024
005be9a
CLDR-17462 main CSS should be loaded in the bundle, not by Java.
srl295 May 30, 2024
3a175b1
CLDR-17462 updates per review comment
srl295 May 30, 2024
e39d2b8
CLDR-17462 workaround css emoji issue
srl295 May 30, 2024
e0a503e
CLDR-17462 right align status cell, use nbsp
srl295 May 30, 2024
12e63df
CLDR-17462 use a zwsp between the inherited mark and the status icon
srl295 May 30, 2024
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
11 changes: 7 additions & 4 deletions tools/cldr-apps/js/src/esm/cldrTable.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,16 +676,19 @@ function getStatusIcon(statusClass) {
switch (statusClass) {
case "approved":
case "contributed":
return "✓"; // U+2713
case "missing":
case "provisional":
case "unconfirmed":
return "✘"; // U+2718
return cldrText.get(`status_${statusClass}`);
case "inherited-provisional":
case "inherited-unconfirmed":
return "↑"; // U+2191
return (
cldrText.get(`status_inherited`) +
"\u202F" +
getStatusIcon(statusClass.split("-")[1])
);
default:
return "?"; // U+003F
return "\ufffd";
Copy link
Member

Choose a reason for hiding this comment

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

This is kind of a non-standard use for U+FFFD; normally it's for encoding errors ( per https://en.wikipedia.org/wiki/Specials_(Unicode_block) ), not software bugs or incompatibilities between front and back ends; not that it matters much, still I think it's not as good as a question mark 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.

that's true, but this is an internal error of some sort.

Copy link
Member

Choose a reason for hiding this comment

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

not an encoding error though; I think it should be reserved for errors in encoding, such as illegal sequences of UTF-8 bytes, or characters from other character sets with no corresponding code in Unicode

}
}

Expand Down
8 changes: 8 additions & 0 deletions tools/cldr-apps/js/src/esm/cldrText.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,14 @@ const strings = {
report_notAcceptable: "Not Acceptable",
report_missing: "Missing",

// for approval status, see VoteResolver.Status
status_approved: "✅\uFE0F",
status_contributed: "☑️",
status_provisional: "✖️",
status_unconfirmed: "❌\uFE0F",
status_missing: "🕳️",
Copy link
Member

Choose a reason for hiding this comment

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

"Hole"? :-)

status_inherited: "⬆️",

special_about: "About Survey Tool",
special_announcements: "Announcements",
special_account: "Account Settings",
Expand Down
10 changes: 6 additions & 4 deletions tools/cldr-apps/js/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
// This file gets bundled into bundle.js’s cldrBundle global
// From there, it is imported by SurveyTool.includeJavaScript()

// global stylesheets
import "./css/cldrForum.css";
import "../../../cldr-code/src/main/resources/org/unicode/cldr/tool/reports.css";

// module stylesheets need to go here. See cldrVue.mjs
// example: import 'someModule/dist/someModule.css'
import "ant-design-vue/dist/antd.min.css";

// global stylesheets
import "./css/cldrForum.css";
import "../../../cldr-code/src/main/resources/org/unicode/cldr/tool/reports.css";
import "../../../cldr-apps/src/main/webapp/surveytool.css";
import "../../../cldr-apps/src/main/webapp/css/redesign.css";

import * as cldrGui from "./esm/cldrGui.mjs";
import * as cldrVue from "./esm/cldrVue.mjs";
import * as cldrMonitoring from "./esm/cldrMonitoring.mjs";
Expand Down
9 changes: 0 additions & 9 deletions tools/cldr-apps/src/main/java/org/unicode/cldr/web/SurveyTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,21 +209,12 @@ private void serveRunnningNormallyPage(

private void includeCss(HttpServletRequest request, PrintWriter out) {
final String contextPath = request.getContextPath();
final String cb = getCacheBustingExtension(request);
out.write(
"<link rel='stylesheet' href='" + contextPath + "/surveytool" + cb + ".css' />\n");
Comment on lines -212 to -214
Copy link
Member Author

Choose a reason for hiding this comment

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

This wasn't working properly, tended to cache the out of date file. Webpack worked correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Great to know webpack now handles all the cache busting!

Does that mean getCacheBustingExtension is now unused, in which case it should be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is used when loading the error and waiting pages still, since the bundle may not be loaded properly.

/*
* Note: cldrForum.css is loaded through webpack
*/
// bootstrap.min.css -- cf. bootstrap.min.js elsewhere in this file
out.write(
"<link rel='stylesheet' href='//stackpath.bootstrapcdn.com/bootswatch/3.1.1/spacelab/bootstrap.min.css' />\n");
out.write(
"<link rel='stylesheet' href='"
+ contextPath
+ "/css/redesign"
+ cb
+ ".css' />\n");
Copy link
Member

Choose a reason for hiding this comment

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

are you sure this isn't still needed by any legacy code, such as jsp?

I searched for "redesign" with a shortcut I like to use:

$ cldrgrep redesign
tools/cldr-apps/js/src/esm/cldrEvent.mjs: * (this is mostly derived from old redesign.js and should be divided
tools/cldr-apps/js/src/esm/cldrInfo.mjs:const ITEM_INFO_ID = "itemInfo"; // must match redesign.css
tools/cldr-apps/js/src/esm/cldrInfo.mjs:const ITEM_INFO_CLASS = "sidebyside-scrollable"; // must match redesign.css, cldrGui.mjs, DashboardWidget.vue
tools/cldr-apps/src/main/webapp/css/redesign.css:/* redesign.css contains a lot of the 'bootstrap' type css,
tools/cldr-apps/src/main/webapp/css/redesign.css: The following is here in redesign.css temporarily!
tools/cldr-apps/src/main/webapp/css/redesign.css: Note: redesign.css is loaded by SurveyTool.java AFTER surveytool.css and bootstrap, so has higher priority;

That confirms it doesn't occur in any jsp files. It points to this comment in redesign.css itself:

/***
 The following is here in redesign.css temporarily!
 Note: redesign.css is loaded by SurveyTool.java AFTER surveytool.css and bootstrap, so has higher priority;
 style.css (for webpack, currently empty) is effectively loaded first, has the lowest priority
***/

body,
html {
	width: 100vw !important;
	height: 100vh !important;
	overflow: hidden !important;
	margin: 0 !important;
	padding: 0 !important;
}

I might have written part of that comment myself a long time ago. What I'm wondering is, does the relative priority between these css files (redesign.css, surveytool.css, bootstrap, style.css) matter, and if it does matter, does this change preserve it?

Copy link
Member

Choose a reason for hiding this comment

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

BTW I have

$ cat ~/bin/cldrgrep
#!/bin/bash
egrep -H "$@" tools/cldr-apps/js/package.json
egrep -H "$@" tools/cldr-apps/js/src/* -d skip
egrep -H "$@" tools/cldr-apps/js/src/esm/*
egrep -H "$@" tools/cldr-apps/js/src/views/*
egrep -H "$@" tools/cldr-apps/src/main/webapp/*.css
egrep -H "$@" tools/cldr-apps/src/main/webapp/css/*.css
egrep -H "$@" tools/cldr-apps/src/main/webapp/*.jsp
egrep -H "$@" tools/cldr-apps/src/main/webapp/WEB-INF/tmpl/*.jsp
egrep -H "$@" tools/cldr-apps/src/main/webapp/WEB-INF/jspf/*.jspf

Copy link
Member Author

Choose a reason for hiding this comment

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

The JSPs don't go through the 'serve normally' route… they would be loaded from their own jsp path.

This change makes sure our css files are loaded last and so can have the final override.

}

private static final String DD_CLIENT_TOKEN = System.getenv("DD_CLIENT_TOKEN");
Expand Down
7 changes: 4 additions & 3 deletions tools/cldr-apps/src/main/webapp/surveytool.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1703,7 +1703,7 @@ The the following can work here:
*/

/* Kashmiri and Urdu are typically in Nastaliq. Kashmiri in particular uses a letter that is
/* Kashmiri and Urdu are typically in Nastaliq. Kashmiri in particular uses a letter that is
not rendered correctly in Noto Naskh (see CLDR-16595) */

:lang(ks), :lang(ur) {
Expand Down Expand Up @@ -2376,8 +2376,9 @@ span.pathChunk:after
}

.statuscell {
text-align: center;
font-weight: bold;
text-align: right;
font-family: emoji !important;
font-variant-emoji: emoji;
}

.voteDiv .winningStatus {
Expand Down
Loading