feat: added functionality to remove saved user preferences by sergeleger · Pull Request #472 · ReadAlongs/Studio-Web

sergeleger · 2025-11-14T18:29:36Z

PR Goal?

Removing stored user preferences required:

to clear browsing data for the site, in which case you also lose your privacy opt-out or opt-in setting for analytics.

this PR adds a "Reset" button to the settings panel that will reset all configurations (including deleting the stored settings).

Fixes?

#462

Feedback sought?

implementation works as intended.
feedback on UI elements in the settings page (new Reset button, and new location of "save preferences" button)
feedback on new En / Fr / Sp messages in i18n/ folder.

Also, additional minor improvements to the settings modal window:

version paragraph had a cursor pointer (giving the impression that it could be clicked) - removed
added dark mode CSS for the language select box, the previous implementation had very low contrast (black text on blue background)
the escape key did not always close the settings window.
- a quick test: click on an empty section of the settings window: escape would not work
language names in select box are now displayed in their native language (English, Francais, Espanol) regardless of the user's current language choice.

Priority?

Medium, non blocker.

Tests added?

Yes

How to test?

Documented steps from the issue #472:

Go to https://readalongs.github.io/Studio-Web/pr-preview/pr-472/
create a readalong
click on the gear and ask for Spanish
click on "Remember my preferences for this site"
Go to https://readalong-studio.mothertongues.org/pr-preview/pr-472/fr/#/
create a readalong
-❗ the web component interface is in Spanish
click on the gear and click on the reset button
the web component should be in French.

Other:

Reset can be used even if no settings have been saved, an easy way to set the defaults.

Confidence?

High.

Version change?

semanticdiff-com · 2025-11-14T18:29:39Z

Review changes with

Changed Files

File	Status
packages/web-component/src/components/read-along-component/read-along.tsx	59% smaller
packages/web-component/src/i18n/messages.fra.json	45% smaller
packages/web-component/src/i18n/messages.eng.json	45% smaller
packages/web-component/src/i18n/messages.spa.json	44% smaller
packages/studio-web/src/assets/bundle.js	Unsupported file format
packages/web-component/cypress/e2e/settings.cy.js	0% smaller
packages/web-component/src/components.d.ts	0% smaller
packages/web-component/src/components/read-along-component/readme.md	Unsupported file format
packages/web-component/src/scss/modules/_settings.scss	Unsupported file format
packages/web-component/src/utils/utils.ts	0% smaller

github-actions · 2025-11-14T18:33:13Z

PR Preview Action v1.6.3
Preview removed because the pull request was closed.
2025-11-21 11:16 UTC

joanise

Works well when I test it.
I have some questions, see below.

I didn't dig enough to understand why most instances of this.X had to be replaced by this.userPreferences.X but it works.

I didn't test if we can still change settings in the html as attributes to the read-along element and have them reflected when you load a readalong. Did you exercise that?

Finally, I'm leaving comments but I'm not confident enough to really review, @deltork I'm hoping you'll do a code review too.

joanise · 2025-11-14T21:56:51Z

   */
-  @Prop({ mutable: true, reflect: true }) scrollBehaviour: ScrollBehaviour =
-    "smooth";
+  @Prop() scrollBehaviour: ScrollBehaviour = "smooth";


Took me a while to figure out why this was still used -- it appears to just be the thing that sets the default. Is this also what gets modified if you set it in the HTML?

Yes, it contains the value passed in the HTML <read-along scroll-behaviour="..." />, if this attribute (or property) is not used, it defaults to "smooth".

Answering the question about this.X vs this.userPreferences.X here also: this.X contains the initial state of the component, either the hard-coded value or the attribute value passed in <read-along />. The previous implementation directly modified this.X overwriting the component's initial state. This refactor makes this.X read-only and it is used only for setting the component's initial state and clearing the users's preferences (see: defaultUserPreferences() where this.X is used) .

joanise · 2025-11-14T22:03:57Z

-      } else if (this.language.match("es") !== null) {
-        this.language = "spa";
+    if (prefs.language.length < 3) {
+      if (prefs.language.match("fr") != null) {


This is weird code. Why not prefs.language !== "fr"? @deltork ? Is this case insensitive?

This allows the app to support both ISO 639-3 and ISO 639-1 ("fra" and "fr") for maximum compatibility.

This makes it case insensitive

Suggested change

if (prefs.language.match("fr") != null) {

if (prefs.language.match(/fr/i) != null) {

sergeleger · 2025-11-17T13:07:28Z

I didn't test if we can still change settings in the html as attributes to the read-along element and have them reflected when you load a readalong. Did you exercise that?

If I'm understanding the use-case correctly something along the lines of?

const el = document.getElementById("readalongInstance");
el.scrollBehaviour = "auto";

Currently would not work. Neither would this:

const el = document.getElementById("readalongInstance");
if(el.scrollBehaviour == "auto") {
...
}

In the first case it would take effect only when/if the user performs a "Reset". In the second, the value returned would always reflect the component's initial state and not the current user defined preference. Both can be addressed.

Is there an example of this or a test?

joanise · 2025-11-17T16:27:09Z

What I meant by "setting the attributes" is right in the HTML itself, someone publishing a readalongs could set language="fra" or the scrolling parameter etc. I think all the use cases are covered by examples in packages/web-component/test-data/ej-fra/*.html so I suppose if CI exercises those, then they're already tested.

joanise · 2025-11-17T16:41:56Z

Thanks for the changes. Looks good to me, but I'll let Del approve once he's done his review.

sergeleger · 2025-11-17T17:14:06Z

Yes, that has been implemented and manually tested. Values passed in HTML become the component's default values when "Reset" is used.

deltork · 2025-11-18T18:35:10Z

I have a minor tweak for the settings UI. see #475.
Otherwise everything looks good to me.

deltork

Please review and merge #475 into this

deltork · 2025-11-17T16:52:31Z

-      } else if (this.language.match("es") !== null) {
-        this.language = "spa";
+    if (prefs.language.length < 3) {
+      if (prefs.language.match("fr") != null) {


This allows the app to support both ISO 639-3 and ISO 639-1 ("fra" and "fr") for maximum compatibility.

deltork · 2025-11-18T16:04:46Z

-      } else if (this.language.match("es") !== null) {
-        this.language = "spa";
+    if (prefs.language.length < 3) {
+      if (prefs.language.match("fr") != null) {


This makes it case insensitive

Suggested change

if (prefs.language.match("fr") != null) {

if (prefs.language.match(/fr/i) != null) {

deltork · 2025-11-18T16:05:05Z

+    if (prefs.language.length < 3) {
+      if (prefs.language.match("fr") != null) {
+        prefs.language = "fra";
+      } else if (prefs.language.match("es") !== null) {


Suggested change

} else if (prefs.language.match("es") !== null) {

} else if (prefs.language.match(/es/i) !== null) {

I see some != with just one equal in there, should they not all be !==?

Could we convert this to a data structure? Regex feels like an over kill and in this case not specific enough (abc_fr_abc would be accepted for French):

const acceptedLanguages = { "en": "eng", "eng": "eng", "fr": "fra", "fra": "fra", "es": "spa", "spa": "spa", } prefs.language = acceptedLanguages[prefs.language.toLowerCase()]||"eng";

That's not quite accurate since the code is in a if block where len<3... which also points to overkill. :)

I like your map, though, seems like a good idea. In Python, [] with an unfound value would raise an exception, here would that return a falsy result instead?

I see some != with just one equal in there, should they not all be !==?

yeh they should be !==

That's not quite accurate since the code is in a if block where len<3... which also points to overkill. :)

Oops, I assumed there was an else clause hiding in all the short diff blocks. Since there are none, there's no 3-character language validation either. But getI18nString handles the bad language input.

I like your map, though, seems like a good idea. In Python, [] with an unfound value would raise an exception, here would that return a falsy result instead?

That's correct, and the || "eng" returns English in those cases (it does not throw an exception).

I'll update the language validation code after Del merges PR #475 (which I've just accepted).

deltork

Looks good to me

sergeleger requested review from deltork and joanise November 14, 2025 18:40

joanise reviewed Nov 14, 2025

View reviewed changes

sergeleger force-pushed the dev.swl/462-clear-user-preferences branch from c03c9c8 to acf6526 Compare November 17, 2025 13:07

sergeleger added 9 commits November 18, 2025 07:57

feat: added user preferences operations

00a7e19

refactor: user preferences do not update component properties

476349e

fix: <select /> elements require either an id or name property

0bcc4ee

fix: removed cursor pointer from version paragraph

3cd7b6e

fix: in dark mode contrast of select boxes was too low

03f4798

fix: escape key press did not always close the settings modal window

7c7a3f8

fix: display language name in their own language

a58fa95

feat: added button to remove saved user preferences

859865f

test: added test for reseting user preferences

027fb9e

sergeleger force-pushed the dev.swl/462-clear-user-preferences branch from 5a7bd60 to 4e5b399 Compare November 18, 2025 11:58

deltork approved these changes Nov 18, 2025

View reviewed changes

deltork and others added 3 commits November 20, 2025 13:32

refactor: restructure settings component layout and styles

6a555bd

refactor: replaced ISO 639-1 support with a lookup object

88955a2

chore: update bundle.js

21491d1

sergeleger force-pushed the dev.swl/462-clear-user-preferences branch from 8902cd5 to 21491d1 Compare November 20, 2025 17:51

sergeleger requested review from deltork and joanise November 20, 2025 17:58

deltork approved these changes Nov 20, 2025

View reviewed changes

sergeleger merged commit 8768407 into main Nov 21, 2025
6 checks passed

sergeleger deleted the dev.swl/462-clear-user-preferences branch November 21, 2025 11:16

joanise mentioned this pull request Dec 10, 2025

If you save web-component preferences, you cannot undo it, and it affects the Studio-Web #462

Closed

	if (prefs.language.match("fr") != null) {
	if (prefs.language.match(/fr/i) != null) {

	} else if (prefs.language.match("es") !== null) {
	} else if (prefs.language.match(/es/i) !== null) {

Uh oh!

Conversation

sergeleger commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Goal?

Fixes?

Feedback sought?

Priority?

Tests added?

How to test?

Confidence?

Version change?

Uh oh!

semanticdiff-com Bot commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joanise left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergeleger commented Nov 17, 2025

Uh oh!

joanise commented Nov 17, 2025

Uh oh!

joanise commented Nov 17, 2025

Uh oh!

sergeleger commented Nov 17, 2025

Uh oh!

deltork commented Nov 18, 2025

Uh oh!

deltork left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergeleger Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deltork left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sergeleger commented Nov 14, 2025 •

edited

Loading

semanticdiff-com Bot commented Nov 14, 2025 •

edited

Loading

github-actions Bot commented Nov 14, 2025 •

edited

Loading

joanise left a comment •

edited

Loading

sergeleger Nov 20, 2025 •

edited

Loading