CLDR-17462 update ST status icons for clarity #3763

srl295 · 2024-05-29T22:18:02Z

CLDR-17462

This PR completes the ticket.

ALLOW_MANY_COMMITS=true

- update missing and inherited

macchiati · 2024-05-30T16:43:31Z

Has this been pushed to staging for UI review?

srl295 · 2024-05-30T16:44:37Z

Has this been pushed to staging for UI review?

Yes and you reviewed it, i'm preparing another push incorporating your feedback (such as the word 'inherited')

- move redesign.css and surveytool.css out of java printfs and into index.js - this gives us the capability for minimization and reduces network loads

srl295 · 2024-05-30T16:53:11Z

how's this for inherited+provisional?

srl295 · 2024-05-30T17:15:09Z

@macchiati i've now updated staging to e39d2b8

macchiati · 2024-05-30T17:15:36Z

how's this for inherited+provisional?

I think that is ok, if you have a narrow space between them. That will allow for narrower columns

srl295 · 2024-05-30T17:18:03Z

how's this for inherited+provisional?

I think that is ok, if you have a narrow space between them. That will allow for narrower columns

maybe a NNBSP

srl295 · 2024-05-30T17:21:00Z

how's this for inherited+provisional?

I think that is ok, if you have a narrow space between them. That will allow for narrower columns

maybe a NNBSP

Chrome, right-aligned, nnbsp

macchiati · 2024-05-30T17:26:42Z

I didn't mean NBSP. That will force all columns on the page to be wide if one of them is. I was thinking the reverse; adding a narrow space (aka thin space) to allow the two icons to be on different lines. Not a non-break one, a breaking one.

srl295 · 2024-05-30T17:42:03Z

I didn't mean NBSP. That will force all columns on the page to be wide if one of them is. I was thinking the reverse; adding a narrow space (aka thin space) to allow the two icons to be on different lines. Not a non-break one, a breaking one.

different lines could be hard to scan, it'll look like alternating content. Here's the result with ZWSP:

srl295 · 2024-05-30T17:43:31Z

tools/cldr-apps/src/main/java/org/unicode/cldr/web/SurveyTool.java

-        final String cb = getCacheBustingExtension(request);
-        out.write(
-                "<link rel='stylesheet' href='" + contextPath + "/surveytool" + cb + ".css' />\n");


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

Great to know webpack now handles all the cache busting!

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

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

macchiati · 2024-05-30T17:54:22Z

Hmmm. Ok, let's try with them on the same line, and we'll ask the TC about it. Note: that only happens when the item being inherited is provisional or worse, right? If it is Contributes or Accepted, we see just a regular ⬆️, right?

stenshamn · 2024-05-30T17:58:51Z

I didn't mean NBSP. That will force all columns on the page to be wide if one of them is. I was thinking the reverse; adding a narrow space (aka thin space) to allow the two icons to be on different lines. Not a non-break one, a breaking one.

different lines could be hard to scan, it'll look like alternating content. Here's the result with ZWSP:

This looks OK to me, FWIW. I agree it's better than causing change in column width.

srl295 · 2024-05-30T17:58:52Z

Hmmm. Ok, let's try with them on the same line, and we'll ask the TC about it. Note: that only happens when the item being inherited is provisional or worse, right? If it is Contributes or Accepted, we see just a regular ⬆️, right?

If I'm reading the logic correctly… only unconfirmed or provisional adds the "inherited" ⬆️. Otherwise it will only show the approved, contributed, missing symbols.

I don't see a code path where it shows only the uparrow.

macchiati · 2024-05-30T18:04:11Z

If that is the case, we don't need the ⬆️ at all, just the X. People will be able to see in the winning cell (from the color) that it is inherited, and the X tells them that it isn't approved/contributed

…

On Thu, May 30, 2024 at 10:59 AM Steven R. Loomis ***@***.***> wrote: Hmmm. Ok, let's try with them on the same line, and we'll ask the TC about it. Note: that only happens when the item being inherited is provisional or worse, right? If it is Contributes or Accepted, we see just a regular ⬆️, right? If I'm reading the logic correctly… only unconfirmed or provisional adds the "inherited" ⬆️. Otherwise it will only show the approved, contributed, missing symbols. I don't see a code path where it shows only the uparrow. — Reply to this email directly, view it on GitHub <#3763 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMAYFZ2C4NLL5VDED63ZE5SHFAVCNFSM6AAAAABIPY5NRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBQGQ4DINBVGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

srl295 · 2024-05-30T18:11:30Z

If that is the case, we don't need the ⬆️ at all, just the X. People will be able to see in the winning cell (from the color) that it is inherited, and the X tells them that it isn't approved/contributed

That's fine, we can drop the inheritance marker symbol then. (Reference to its discussion: CLDR-11103 )

srl295 · 2024-05-30T19:50:03Z

So, thoughts? drop the inheritance icon?

macchiati · 2024-05-30T20:16:46Z

Let's leave it in for now, so we don't delay.

srl295 · 2024-05-30T20:22:52Z

@macchiati just to be really clear, you are approving

A: NNBSP e0a503e - this is the current code in the PR

(updated image)

B: `e39d2b8` what is currently on cldr-staging

C: (not committed) using a ZWSP instead of NNBSP

srl295 · 2024-05-30T20:25:19Z

Let's leave it in for now, so we don't delay.

let me know A B or C and i'll accept the ticket and get ready to merge

macchiati · 2024-05-30T20:32:46Z

Ok, if C: (not committed) using a ZWSP instead of NNBSP is ready to go, let's do that.

srl295 · 2024-05-30T20:43:32Z

@macchiati it's option C now

AEApple · 2024-05-30T20:50:51Z

Looks nice!

btangmu

I have some doubts about getCacheBustingExtension and redesign.css, see my comments on those; otherwise looks fine

btangmu · 2024-05-30T20:50:06Z

tools/cldr-apps/js/src/esm/cldrTable.mjs

    default:
-      return "?"; // U+003F
+      return "\ufffd";


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

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

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

btangmu · 2024-05-30T20:51:55Z

tools/cldr-apps/js/src/esm/cldrText.mjs

+  status_contributed: "☑️",
+  status_provisional: "✖️",
+  status_unconfirmed: "❌\uFE0F",
+  status_missing: "🕳️",


"Hole"? :-)

btangmu · 2024-05-30T20:55:17Z

tools/cldr-apps/src/main/java/org/unicode/cldr/web/SurveyTool.java

-        final String cb = getCacheBustingExtension(request);
-        out.write(
-                "<link rel='stylesheet' href='" + contextPath + "/surveytool" + cb + ".css' />\n");


Great to know webpack now handles all the cache busting!

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

btangmu · 2024-05-30T21:03:15Z

tools/cldr-apps/src/main/java/org/unicode/cldr/web/SurveyTool.java

-                        + contextPath
-                        + "/css/redesign"
-                        + cb
-                        + ".css' />\n");


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?

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

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.

CLDR-17462 update ST status icons for clarity

154c842

srl295 requested review from macchiati, AEApple, cgaarden, stenshamn and btangmu May 29, 2024 22:18

srl295 self-assigned this May 29, 2024

srl295 marked this pull request as ready for review May 29, 2024 22:18

CLDR-17462 update ST status icons for clarity

4a10a6c

- update missing and inherited

CLDR-17462 main CSS should be loaded in the bundle, not by Java.

005be9a

- move redesign.css and surveytool.css out of java printfs and into index.js - this gives us the capability for minimization and reduces network loads

srl295 added 2 commits May 30, 2024 11:57

CLDR-17462 updates per review comment

3a175b1

CLDR-17462 workaround css emoji issue

e39d2b8

CLDR-17462 right align status cell, use nbsp

e0a503e

srl295 commented May 30, 2024

View reviewed changes

macchiati previously approved these changes May 30, 2024

View reviewed changes

CLDR-17462 use a zwsp between the inherited mark and the status icon

12e63df

srl295 dismissed macchiati’s stale review via 12e63df May 30, 2024 20:34

srl295 requested a review from macchiati May 30, 2024 20:43

macchiati approved these changes May 30, 2024

View reviewed changes

srl295 merged commit f9b3376 into unicode-org:main May 30, 2024
11 checks passed

srl295 deleted the cldr-17462/icons branch May 30, 2024 21:06

btangmu approved these changes May 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLDR-17462 update ST status icons for clarity #3763

CLDR-17462 update ST status icons for clarity #3763

srl295 commented May 29, 2024 •

edited

Loading

macchiati commented May 30, 2024

srl295 commented May 30, 2024

srl295 commented May 30, 2024

srl295 commented May 30, 2024

macchiati commented May 30, 2024

srl295 commented May 30, 2024

srl295 commented May 30, 2024

macchiati commented May 30, 2024 •

edited

Loading

srl295 commented May 30, 2024

srl295 May 30, 2024

btangmu May 30, 2024

srl295 May 30, 2024

macchiati commented May 30, 2024

stenshamn commented May 30, 2024

srl295 commented May 30, 2024 •

edited

Loading

macchiati commented May 30, 2024 via email

srl295 commented May 30, 2024

srl295 commented May 30, 2024

macchiati commented May 30, 2024

srl295 commented May 30, 2024 •

edited

Loading

srl295 commented May 30, 2024

macchiati commented May 30, 2024

srl295 commented May 30, 2024

AEApple commented May 30, 2024

btangmu left a comment

btangmu May 30, 2024

srl295 May 30, 2024

btangmu May 30, 2024

btangmu May 30, 2024

btangmu May 30, 2024

btangmu May 30, 2024

btangmu May 30, 2024

srl295 May 30, 2024

CLDR-17462 update ST status icons for clarity #3763

CLDR-17462 update ST status icons for clarity #3763

Conversation

srl295 commented May 29, 2024 • edited Loading

macchiati commented May 30, 2024

srl295 commented May 30, 2024

srl295 commented May 30, 2024

srl295 commented May 30, 2024

macchiati commented May 30, 2024

srl295 commented May 30, 2024

srl295 commented May 30, 2024

macchiati commented May 30, 2024 • edited Loading

srl295 commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macchiati commented May 30, 2024

stenshamn commented May 30, 2024

srl295 commented May 30, 2024 • edited Loading

macchiati commented May 30, 2024 via email

srl295 commented May 30, 2024

srl295 commented May 30, 2024

macchiati commented May 30, 2024

srl295 commented May 30, 2024 • edited Loading

A: NNBSP e0a503e - this is the current code in the PR

B: e39d2b8 what is currently on cldr-staging

C: (not committed) using a ZWSP instead of NNBSP

srl295 commented May 30, 2024

macchiati commented May 30, 2024

srl295 commented May 30, 2024

AEApple commented May 30, 2024

btangmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srl295 commented May 29, 2024 •

edited

Loading

macchiati commented May 30, 2024 •

edited

Loading

srl295 commented May 30, 2024 •

edited

Loading

srl295 commented May 30, 2024 •

edited

Loading

B: `e39d2b8` what is currently on cldr-staging