feat: add rtl/ltr direction support for adding instrument name #448

PouyaMohseni · 2025-12-03T18:49:00Z

yinanazhou · 2025-12-04T17:17:48Z

web-app/django/VIM/templates/instruments/includes/addName.html

      {
          "wikidata_code": "{{ language.wikidata_code }}",
          "autonym": "{{ language.autonym }}",
-          "en_label": "{{ language.en_label }}"


I just noticed that this template file has a inline script. This is quite messy and not ideal. Can we get rid of it in this PR? Thanks.

Yeah, I just took a closer look at this...

If we want to keep this rendered server-side, then probably the cleanest thing to do is pass a JSON string to the context and then load it here.

Just make the languages context object into a python object (list of dictionaries), stringify it, and then assign it directly to a variable.

It's also the "Django" way: see the json_script template filter: https://docs.djangoproject.com/en/5.2/ref/templates/builtins/#json-script

dchiller · 2025-12-04T21:47:08Z

web-app/django/VIM/templates/instruments/includes/addName.html

      {
          "wikidata_code": "{{ language.wikidata_code }}",
          "autonym": "{{ language.autonym }}",
-          "en_label": "{{ language.en_label }}"


Yeah, I just took a closer look at this...

If we want to keep this rendered server-side, then probably the cleanest thing to do is pass a JSON string to the context and then load it here.

Just make the languages context object into a python object (list of dictionaries), stringify it, and then assign it directly to a variable.

dchiller · 2025-12-04T22:09:16Z

web-app/frontend/src/instruments/AddName.ts

+
+const languages = readLanguagesFromPage();
+
+export { languages };


Is this intended to be used elsewhere? Especially since the export having any useful data depends on the languages being templated into the html...if we were populating this with an api call, I would understand exporting just because, but as it stands now, I think it's clearer to not export (if someone wants this language object later somewhere else, the fact that it isn't exported will be a good hint that it's not self-contained)

yinanazhou · 2025-12-05T02:06:50Z

I've done the same thing here for instruments_chart_data as an example:

UMIL/web-app/django/VIM/templates/main/index.html

Line 176 in cea2f93

    
           <script type="application/json" id="instruments-chart-data">{{ instruments_chart_data|safe }}</script>

UMIL/web-app/django/VIM/apps/main/views.py

Lines 34 to 51 in cea2f93

    
           instruments_chart_data = [] 
        
           for instrument in top_instruments_by_languages: 
        
               # Get the English name if available, otherwise use the first available name 
        
               try: 
        
                   english_name = instrument.instrumentname_set.filter( 
        
                       language__en_label="English" 
        
                   ).first() 
        
                   name = ( 
        
                       english_name.name 
        
                       if english_name 
        
                       else instrument.instrumentname_set.first().name 
        
                   ) 
        
               except: 
        
                   name = f"Instrument {instrument.wikidata_id}" 
        
               instruments_chart_data.append( 
        
                   {"name": name, "count": instrument.language_count} 
        
               )

yinanazhou · 2025-12-05T02:29:26Z

Forgot to include, you can then parse data to frontend like this:

UMIL/web-app/frontend/src/stats/BarCharts.ts

Lines 29 to 39 in cea2f93

    
           const instrumentsDataElement = document.getElementById( 
        
             'instruments-chart-data', 
        
           ); 
        
           const languagesDataElement = document.getElementById( 
        
             'languages-chart-data', 
        
           ); 
        
           if (!instrumentsDataElement || !languagesDataElement) { 
        
             console.error('Chart data elements not found'); 
        
             return; 
        
           }

yinanazhou · 2025-12-09T21:29:36Z

web-app/frontend/src/instruments/AddName.ts


-declare const languages: WikidataLanguage[];
+// Helper to read JSON from the template-inserted script tag
+function readLanguagesFromPage() {


This function is only called once? Does it need to be a function?

I thought it would be easier to reuse it in the future, as we want to have a page to add new instruments, which would need such functionality.

In that case, I would suggest to put it into a file named utils.ts and name the function getLanguages.

yinanazhou · 2025-12-09T21:31:37Z

web-app/frontend/src/instruments/helpers/AddNameManager.ts

+      );
+
+      if (lang) {
+        nameInput.setAttribute('dir', lang.html_direction);


You are setting the text alignment below. Is this needed?

On my device, text alignment was sufficient, but I wasn’t aware of the other behaviors, so I added it just to be safe. If you recommend otherwise, I remove it.

To make it more clear, I suggest you use css class instead.

Suggested change

nameInput.setAttribute('dir', lang.html_direction);

nameInput.classList.add(lang.html_direction === 'rtl' ? 'force-rtl' : 'force-ltr');

dchiller

I'm good once @yinanazhou is...

…guages`

yinanazhou · 2025-12-12T14:49:35Z

web-app/frontend/src/instruments/helpers/AddNameManager.ts

+          lang.html_direction === 'rtl' ? 'force-rtl' : 'force-ltr',
+        );
+        nameInput.style.textAlign =
+          lang.html_direction === 'rtl' ? 'right' : 'left';


Is this still needed?

Yes, it doesn't work on my side without the second part.

I think it's better to make .force-rtl and .force-ltr work than doing this. Just had a closer look at them, you can try move them out of the nest they are in right now.

@yinanazhou, Do you approve the changes?

yinanazhou · 2025-12-19T20:47:33Z

web-app/frontend/src/instruments/helpers/AddNameManager.ts

+      );
+
+      if (lang) {
+        nameInput.classList.remove('force-rtl', 'force-ltr');


This is a weird place to clean up the class. You should always clean up when the language changes. Move this outside of if and up to make it the first thing to do when the change happens. Also remember to trigger event in restoreFormData() after L307 to make sure the change event gets fired.

feat: add rtl/ltr direction support for adding instrument name

0ffed3b

PouyaMohseni force-pushed the rtl-support-add-inst branch from e1f6578 to 0ffed3b Compare December 3, 2025 23:50

PouyaMohseni marked this pull request as ready for review December 3, 2025 23:56

dchiller approved these changes Dec 4, 2025

View reviewed changes

yinanazhou reviewed Dec 4, 2025

View reviewed changes

dchiller requested changes Dec 4, 2025

View reviewed changes

dchiller reviewed Dec 4, 2025

View reviewed changes

fix: remove inline script from addName.html

6395f39

PouyaMohseni force-pushed the rtl-support-add-inst branch from 627b8fc to 6395f39 Compare December 7, 2025 02:29

yinanazhou reviewed Dec 9, 2025

View reviewed changes

dchiller approved these changes Dec 10, 2025

View reviewed changes

refactor: move readLanguagesFromPage to utils and rename to `getLan…

b72ad85

…guages`

yinanazhou reviewed Dec 12, 2025

View reviewed changes

PouyaMohseni force-pushed the rtl-support-add-inst branch from 3acc61e to 598d2b9 Compare December 14, 2025 21:35

yinanazhou requested changes Dec 19, 2025

View reviewed changes

PouyaMohseni mentioned this pull request Dec 20, 2025

fix: add notranslate support to some UI components #462

Open

PouyaMohseni added 2 commits December 19, 2025 20:56

feat: move .force-ltr and .force-rtl out of the nest

ca95f95

fix: make force-rtl and force-ltr handle alignment

9ae269e

PouyaMohseni force-pushed the rtl-support-add-inst branch from 598d2b9 to 9ae269e Compare December 20, 2025 22:14


		const languages = readLanguagesFromPage();

		export { languages };

	nameInput.setAttribute('dir', lang.html_direction);
	nameInput.classList.add(lang.html_direction === 'rtl' ? 'force-rtl' : 'force-ltr');

feat: add rtl/ltr direction support for adding instrument name #448

Are you sure you want to change the base?

feat: add rtl/ltr direction support for adding instrument name #448

Uh oh!

Conversation

PouyaMohseni commented Dec 3, 2025

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!

yinanazhou commented Dec 5, 2025

Uh oh!

yinanazhou commented Dec 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yinanazhou Dec 10, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dchiller 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!

yinanazhou Dec 12, 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!

yinanazhou Dec 19, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yinanazhou Dec 10, 2025 •

edited

Loading

yinanazhou Dec 12, 2025 •

edited

Loading

yinanazhou Dec 19, 2025 •

edited

Loading