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

CLDR-16835 DDL announcement #3796

Merged
merged 2 commits into from
Aug 21, 2024
Merged

CLDR-16835 DDL announcement #3796

merged 2 commits into from
Aug 21, 2024

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Jun 11, 2024 •
edited
Loading

(Note: waiting for #3795 )

CLDR-16835

  • Announcement UI to announce to Non-TC locales, and/or Non-TC orgs

  • This PR completes the ticket.

Supports CLDR-16614

ALLOW_MANY_COMMITS=true

Example view of the announcements panel

image

Example view of posting

image

@macchiati
Copy link
Member

What does ! mean in the Locales field? Is that a magic character?

@srl295
Copy link
Member Author

srl295 commented Jun 12, 2024

What does ! mean in the Locales field? Is that a magic character?

Magic for now but could be a button in the UI.

@macchiati
Copy link
Member

macchiati commented Jun 12, 2024 via email

@srl295
Copy link
Member Author

srl295 commented Jun 12, 2024

Could you for now just add a text note below the box that has the magic

symbols and what they mean?

On Tue, Jun 11, 2024 at 5:01 PM Steven R. Loomis @.***>

wrote:

What does ! mean in the Locales field? Is that a magic character?

Magic for now but could be a button in the UI.

—

Reply to this email directly, view it on GitHub

#3796 (comment),

or unsubscribe

https://github.com/notifications/unsubscribe-auth/ACJLEME6DQ3XO262WSDA22TZG6FUNAVCNFSM6AAAAABJEUSTUCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRRHAYTEOBZHE

.

You are receiving this because you commented.Message ID:

@.***>

There's a note in the box, see other comment. Does that work?

@srl295 srl295 mentioned this pull request Jun 12, 2024
Merged
1 task
@macchiati
Copy link
Member

macchiati commented Jun 12, 2024 via email

@srl295
Copy link
Member Author

srl295 commented Jun 12, 2024

Optional list of locales (like: aa fr zh) (fr implies fr_CA/etc.) (empty for all locales, '!' for non-TC)
A bit too telegraphic. If you haven't merged yet, I suggest: => If empty, all locales; If !, then all non-TC locales; otherwise a specific list, eg: aa fr zh. Note: fr means all sublocales:

@macchiati i do appreciate the feedback. and it makes sense.

Timing wise, this can't merge until #3795 merges. So it will be out for review again once that has happened.

@srl295 srl295 force-pushed the cldr-16835/ddl-announcement branch from 89384a0 to 8add514 Compare June 13, 2024 16:35
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • tools/cldr-apps/js/src/esm/cldrLoad.mjs is no longer changed in the branch
  • tools/cldr-apps/js/src/esm/cldrStatus.mjs is no longer changed in the branch
  • tools/cldr-apps/js/src/views/MainHeader.vue is no longer changed in the branch
  • tools/cldr-apps/src/main/java/org/unicode/cldr/util/CLDRConfigImpl.java is no longer changed in the branch
  • tools/cldr-apps/src/main/java/org/unicode/cldr/web/DataPage.java is no longer changed in the branch
  • tools/cldr-apps/src/main/java/org/unicode/cldr/web/SurveyAjax.java is no longer changed in the branch
  • tools/cldr-apps/src/main/java/org/unicode/cldr/web/SurveyMain.java is no longer changed in the branch
  • tools/cldr-apps/src/main/java/org/unicode/cldr/web/UserRegistry.java is no longer changed in the branch
  • tools/cldr-code/src/main/java/org/unicode/cldr/test/SubmissionLocales.java is no longer changed in the branch
  • tools/cldr-code/src/main/java/org/unicode/cldr/util/CLDRConfig.java is no longer changed in the branch
  • tools/cldr-code/src/test/java/org/unicode/cldr/test/TestSubmissionLocales.java is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@srl295 srl295 changed the title 🚧 CLDR-16835 DDL announcement CLDR-16835 DDL announcement Jun 13, 2024
@srl295 srl295 requested review from btangmu and macchiati June 13, 2024 16:38
@srl295 srl295 marked this pull request as ready for review June 13, 2024 16:38
@srl295
Copy link
Member Author

srl295 commented Jun 13, 2024

@btangmu @macchiati this is now ready. Note it is TC locales, not extended submission locales.

macchiati
macchiati requested changes Jun 13, 2024
View reviewed changes
Copy link
Member

@macchiati macchiati left a comment •
edited by srl295
Loading

Choose a reason for hiding this comment

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

@btangmu @macchiati this is now ready. Note it is TC locales, not extended submission locales.

That statement is not clear. I assume that you are saying:

  1. non-TC locales will allow submission during vetting.
  2. The extended version is not included, whereby some TC locales (on exception list or with only one active TC member) will also allow submission during vetting.
  3. You would like to submit this PR first, and do #2 afterwards.

Can you confirm that this is the case, or provide a correction to the above.

@macchiati macchiati self-requested a review June 13, 2024 17:29
macchiati
macchiati previously approved these changes Jun 13, 2024
View reviewed changes
Copy link
Member

@macchiati macchiati left a comment

Choose a reason for hiding this comment

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

Whoops, misread this as pertaining to the other ticket.

I think this is an advance, but we will probably want the ability to send to these groups as well. (we can probably work around it for now.)

  1. Vetters with at least one locale that allows submission during vetting
  2. Vetters without at least one locale that allows submission during vetting

@srl295
Copy link
Member Author

srl295 commented Jun 13, 2024

Whoops, misread this as pertaining to the other ticket.

Yes, I can confirm that! And apologies, because of how we use merges, this PR had unrelated commits in it earlier while a draft, so that makes it confusing.

I had put this PR out there for UI review.

I think this is an advance, but we will probably want the ability to send to these groups as well. (we can probably work around it for now.)

  1. Vetters with at least one locale that allows submission during vetting
  2. Vetters without at least one locale that allows submission during vetting

Good idea.

In the short term (even without this PR) that can be accomplished by pasting an explicit list into the 'locales' list. I did confirm that the database has a limit on how many locales can be put there! looks like 122 characters (space separated) of locales.

@srl295
Copy link
Member Author

srl295 commented Jun 13, 2024

@btangmu want to review the js / vue ?

btangmu
btangmu previously approved these changes Jun 13, 2024
View reviewed changes
Copy link
Member

@btangmu btangmu left a comment

Choose a reason for hiding this comment

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

Seems like the terminology should be made more consistent, either "non-TC" or "DDL" but not both if they're supposed to have the same implications.

Ideally the string "!" should be more encapsulated.

Bypassing LocaleSet and LocaleNormalizer, as far as "!" is concerned, also makes me a little uneasy, and goes along with a general feeling of "creeping featurism". It's in a good cause, though, and I don't have any alternative to suggest.

tools/cldr-apps/js/src/views/AnnouncePost.vue Show resolved Hide resolved
tools/cldr-apps/js/src/views/AnnounceForm.vue Outdated Show resolved Hide resolved
tools/cldr-apps/src/main/java/org/unicode/cldr/web/AnnouncementData.java Show resolved Hide resolved
@srl295
Copy link
Member Author

srl295 commented Jun 13, 2024

image

UI idea.. the locales box goes away if 'All' or 'DDL' is chosen.

@macchiati
Copy link
Member

macchiati commented Jun 13, 2024 via email

@srl295
CLDR-16835 update form for sending announcements
ec3774a 
- radio button for all, ddl, choose locales
- validation, where the Post button doesn't show if validation fails.
- documentation of ddl vs non-ddl locales
@srl295 srl295 dismissed stale reviews from btangmu and macchiati via ec3774a June 14, 2024 23:14
@srl295 srl295 requested review from macchiati and btangmu June 14, 2024 23:14
@srl295
Copy link
Member Author

srl295 commented Jun 14, 2024

image UI idea.. the locales box goes away if 'All' or 'DDL' is chosen.

Updated as above. Also the Post button is dimmed if body, subject, locales aren't filled in properly.

Feel free to review. Let's consider merge AFTER Monday's festivities, perhaps with a stint on cldr-staging.

macchiati
macchiati approved these changes Jun 18, 2024
View reviewed changes
Copy link
Member

@macchiati macchiati left a comment

Choose a reason for hiding this comment

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

Looks ok for the java; haven't looked at the mjs/vue

@srl295 srl295 merged commit 2049fef into unicode-org:main Aug 21, 2024
11 checks passed
@srl295 srl295 deleted the cldr-16835/ddl-announcement branch August 21, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@macchiati macchiati macchiati approved these changes

@btangmu btangmu Awaiting requested review from btangmu

Assignees

@srl295 srl295

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@srl295 @macchiati @btangmu