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.

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

Fix: losing context when changing language in concepts page #642

Merged
merged 59 commits into from
Aug 26, 2024

Conversation

Bilelkihal
Copy link
Collaborator

@Bilelkihal Bilelkihal commented May 30, 2024

Related issues:

#557
#617
agroportal/project-management#518

Description

To see the problem:
1- Pick concept 'A' from the tree view.
2- Change language to French for example.
3- Change language to All languages.
4- Choose concept 'B'.
5- Go back to French.
Now, instead of showing concept 'B' in French, it goes back to showing 'A'.

Technical Cause

Though the exact issue remains somewhat elusive, here's how it unfolds: when the language is changed, the select component triggers an event called 'lang_changed'. This event is intercepted to update the URL via the history stimulus controller, and refresh the content frame via the turbo frame controller
action: "lang_changed->history#updateURL lang_changed->turbo-frame#updateFrame"

In attempting to trace the execution flow leading to the anomaly, I encountered an unusual occurrence within the history stimulus controller. While debugging the JavaScript line by line, I observed an abrupt change in window.location.search to an incorrect concept.

Justification of the Technical Choice

I opted to refresh the page each time the language changes, but using Turbo to do it without reloading the whole browser.
Benifits:

  • Issue resolved
  • Performance remains the same because we were refreshing the bottom part of the page anyway, while keeping the header.
  • The code for changing languages is now simpler and in one place.

@Bilelkihal Bilelkihal added the bug Something isn't working label May 30, 2024
@Bilelkihal Bilelkihal self-assigned this May 30, 2024
Copy link
Collaborator Author

@Bilelkihal Bilelkihal left a comment

Choose a reason for hiding this comment

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

Copy link
Collaborator

@syphax-bouazzouni syphax-bouazzouni left a comment

Choose a reason for hiding this comment

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

the fix seems fine by me, simplicity is always better.
But can you just add a video, I want to see if refresh page doesn't degrade the UX of refreshing only content part.

@syphax-bouazzouni
Copy link
Collaborator

Great PR description, btw.

@syphax-bouazzouni syphax-bouazzouni self-requested a review June 3, 2024 12:31
Copy link
Collaborator

@syphax-bouazzouni syphax-bouazzouni left a comment

Choose a reason for hiding this comment

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

After testing it, it did not work

Enregistrement.de.l.ecran.2024-06-03.a.14.31.33.mov

@Bilelkihal
Copy link
Collaborator Author

After testing it, it did not work

Enregistrement.de.l.ecran.2024-06-03.a.14.31.33.mov

It's weird that u r in the Animal husbandry and breeding class without having it in the params of your browser URL.
anyway, this is then a whole different issue

@Bilelkihal
Copy link
Collaborator Author

@syphax-bouazzouni how did you arrive to this class?

@syphax-bouazzouni
Copy link
Collaborator

syphax-bouazzouni commented Jun 3, 2024

After testing it, it did not work
Enregistrement.de.l.ecran.2024-06-03.a.14.31.33.mov

It's weird that u r in the Animal husbandry and breeding class without having it in the params of your browser URL. anyway, this is then a whole different issue

To reproduce:

  • went to INREATHES concepts tabs
  • Open one the tree of the concept "02. AGRI....."
  • Clicked on the "Animal husbandry...",
  • Then tried to change the language.
  • lost the context

@Bilelkihal
Copy link
Collaborator Author

Yes, you are right, I broke the original behavior when I tried to clean the old code of the language selector, now it's working:

Enregistrement.de.l.ecran.2024-06-03.a.16.45.47.mov

@Bilelkihal
Copy link
Collaborator Author

Yes, you are right, I broke the original behavior when I tried to clean the old code of the language selector, now it's working:

Enregistrement.de.l.ecran.2024-06-03.a.16.45.47.mov

I also tested the case:
1- Pick concept 'A' from the tree view.
2- Change language to French for example.
3- Change language to All languages.
4- Choose concept 'B'.
5- Go back to French.

and it's working correclty

@syphax-bouazzouni
Copy link
Collaborator

Yes, you are right, I broke the original behavior when I tried to clean the old code of the language selector, now it's working:

Enregistrement.de.l.ecran.2024-06-03.a.16.45.47.mov

From this video, I see two other issues:

  • The UX is less good, the progress is full, but you wait 2-5s seconds after to see the content, without anything telling you that it is loading.
  • The split component is rendered each time you refresh, I think
    image

@Bilelkihal
Copy link
Collaborator Author

Yes, you are right, I broke the original behavior when I tried to clean the old code of the language selector, now it's working:
Enregistrement.de.l.ecran.2024-06-03.a.16.45.47.mov

From this video, I see two other issues:

  • The UX is less good, the progress is full, but you wait 2-5s seconds after to see the content, without anything telling you that it is loading.
  • The split component is rendered each time you refresh, I think
    image

Thanks for ur remark, then the only solution is to refrech the whole page:

Enregistrement.de.l.ecran.2024-06-03.a.17.02.44.mov

PS. my dev environment is slower in general it's not related to this

@syphax-bouazzouni
Copy link
Collaborator

Nah refreshing all the pages, is old school.

The old way of refreshing only the frame was better, even if it is more complex. You just need to know why we did lose the context and fix only that.

@Bilelkihal
Copy link
Collaborator Author

Nah refreshing all the pages, is old school.

The old way of refreshing only the frame was better, even if it is more complex. You just need to know why we did lose the context and fix only that.

I did already spend a whole day to figure out, but there's nothing and the code was so complicated anyway and it need to be simplified

@syphax-bouazzouni syphax-bouazzouni marked this pull request as draft June 3, 2024 15:30
@syphax-bouazzouni
Copy link
Collaborator

@Bilelkihal postpone this, will go back to it in the future.
You can deploy this version (branch) to TestPortal, if you want to see it in production, maybe it is good enough.

Bilelkihal and others added 5 commits June 14, 2024 11:42
* fix xml download button in summary page, export metadata

* adjust the position of the export metadata download button and make the hover cursor pointer for it
…ab (#539)

* refactor concepts json code and put it in a stimulus controller

* adjust the position of the concepts json link

* remove non related code to concepts json PR

* remove non related code to contextual json pull request

* pass base class URL directly to concepts json stimulus controller

* clean concepts json stimulus code

* remove undesired code from concepts json button PR

* fix typo in agent_search_input_component.html.haml

* rename concepts json button stimulus controller

* clean concepts json button controller code

* clean concepts json button related code

---------

Co-authored-by: Syphax bouazzouni <gs_bouazzouni@esi.dz>
Co-authored-by: Bilel KIHAL <kb_kihal@esi.dz>
Co-authored-by: @SirineMhedhbi
syphax-bouazzouni and others added 13 commits July 17, 2024 10:15
* add cookies action to show and save cookie state in a session

* add link button component helper

* add cookie modal view

* enhance the style of the cookies banner

---------

Co-authored-by: Bilel KIHAL <kb_kihal@esi.dz>
…tip (#713)

* update summary page categories display to show acronym with tooltip

* fix submission flow system tests after changing their display

* make show_category return the full domain if not a category

* update the show category name to use clickable state if not a category and is a link
* make agents search field component results unlimited to only 4

* make agents cherchable by acronym, affiliations, email and home page

* Update the agent search input to display the result in this format Orga name (ACRONYM) – ROR:003vg9w96 and for persons Person Name – ORCID:0000-1524-5487-5487

* chagne display mode value type from string to boolean in search input component

* remove agents search logic from UI side and limit it to only name, acronym and identifiers

* use the endpoint '/search/agents' for the agents search input component
* Hide affiliations when the agent is an organization in the agents form

* Change the name of the function affliations? to is_organization? to make it more clear

* fix submission flows test after the fix of the PR #713

* fix agent tests after changing the behavior of displaying affiliations

---------

Co-authored-by: Syphax <gs_bouazzouni@esi.dz>
…type (#721)

* remove the restriction for certain submission fields to create only organization type or person type agents

* make orcid or ror default input based on agent type in agent form

* fix disappeared signup form icons

* fix nested agents creation

* fix agent form not filling saved identifier values

* create an agent identifier input helper

* disable cookie banner in development and test  mode

* fix agent tests after enforcing ROR for organizations

---------

Co-authored-by: Syphax <gs_bouazzouni@esi.dz>
* Fix propertyId param auto added to summary page URL

* remove added PropertyId param to the URL when opening summary page

* Fix auto added propertyId to the summary page URL

* Fix auto added instanceid to the summary page URL

* Fix auto added propertyId to the summary page URL

* add back the constraint of making frames loading lazy while env.development

* make by default properties section displays the first property

* make by default instances section displays the first instance

* change the name of the function instances_tree_first_id by search_first_instance_id

* add a clarification comment in search_first_instance_id method

* move get_property function to properties controller

---------

Co-authored-by: Syphax <gs_bouazzouni@esi.dz>
@syphax-bouazzouni syphax-bouazzouni marked this pull request as ready for review August 5, 2024 12:21
Copy link
Collaborator

@syphax-bouazzouni syphax-bouazzouni left a comment

Choose a reason for hiding this comment

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

updated the code to fix the issue of not saving the good current URL

@Bilelkihal
Copy link
Collaborator Author

Bilelkihal commented Aug 5, 2024

Not working for the initial use case issue:
1- Pick concept 'A' from the tree view.
2- Change language to French for example.
3- Change language to All languages.
4- Choose concept 'B'.
5- Go back to French.
Now, instead of showing concept 'B' in French, it goes back to showing 'A'.

Copy link
Collaborator Author

@Bilelkihal Bilelkihal left a comment

Choose a reason for hiding this comment

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

Working ✅

@syphax-bouazzouni syphax-bouazzouni merged commit 7647929 into development Aug 26, 2024
4 checks passed
@user34ngrau
Copy link

user34ngrau commented Sep 5, 2024

Q&A session - 5th September (Anne sophie & Nina)
Template used: Number - [Title of the issue] : [Description of the issue] followed by an example : text and/ or screenshot and/or recording”

1-[Langue des catégories du filtre] : [Quand on met la langue du site en francais les sous categories du filtre "Langues naturelles", "niveaux de formalité" restent en anglais]
image

2- [Selection d'ontologies multilangues] : [Pas de possibilité de faire un regroupement/intersection d'ontologies qui comprennent plusieurs langues]
Ex: je souhaite voir des ontologies qui sont en francais ET en anglais, or je n'obtiens que les ontologies qui ont soit une des langues ou l'autre
https://github.com/user-attachments/assets/da551229-289a-4996-847f-4ff1554abc88

3-[Langue de définition des classes] : [La definition d'une classe est décrite en anglais alors que la langue du contenu est le francais]
Ex: AHOL ontology
image

4-[Langue de synonymes des classes] : [Pas de synonmye francais pour certaines classes]
Ex: GAO ontology
image

5-[Orthographe de synonymes des classes] : [Pas d'harmonisation de notation des synonymes]
Ex: GAO ontology, certains termes ont des majuscules et d'autres en minuscules
image

6-[Labélisation nom préféré pour synonyme des classes] : [Pas label de "nom préféré" pour des synonymes]
Ex: GAO ontology, pas d'indication du "nom préféré" pour le synonyme "shoot apex"
image

7-[Classement des noms préférés des classes] : [En fonction des classes selectionnées, la langue préférée qui apparait en premier varie]
Ex: GAO ontology, pour la classe "reproductive organes", le nom préféré est en anglais alors que pour "vegetative organs" cest le terme francais qui apparait en premier
https://github.com/user-attachments/assets/d2366ac8-4d42-44ad-bc06-0182f7143743

8-[Ordre alphabétique et "All language"] : [Le classement des "Classes" nest plus dans l'ordre alphabétique quand la langue du contenu est "All language"]
Ex: GAO ontology, Banana ontology
image

9-[Changement du nom des classes en chiffre] : [Quand change la langue du contenu, le nom des classes devient des chiffres]
Ex: GAO ontology: anglais a francais "ovaries" en "gao:30030",
https://github.com/user-attachments/assets/78b63e72-fda6-4225-b850-b2a8c7452ce0
et pour MDRFRE ontology: "all language" noms en francais et chiffre en "anglais" (pas de langue française proposée"
https://github.com/user-attachments/assets/1a57268d-d5c0-4ae1-a305-5dac1abadda5

10- [Ordre des classes avec accent] : [Les classes avec des accents ne sont pas classé par ordre alphabétique, se retrouve en fin de liste]
Ex: GAO ontology , "écorce" est à la fin
image

11- [XX] : [XX]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants