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

Add tabbed TranslatedContent form #1427

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

BeritJanssen
Copy link
Collaborator

Close #1321; close #1399.

This branch adds a custom template for TranslatedContent models, which makes sure we can add/remove translations inline.

Screen.Recording.2024-12-13.at.13.41.58.mov

@BeritJanssen
Copy link
Collaborator Author

NB: I've also experimentally disabled the initCollapsibleInlineForms in experiment_form.js, and instead added the ['wide', 'collapse'] classes to the Inline classes. Let's compare what we like best after merging. I do think the custom solution looks nicer, but perhaps we can achieve this with styling rather than javascript?

@drikusroor
Copy link
Contributor

Just a quick comment on what I see directly in the video, not code-related per se:

  • The language is a bit hard to read on a (dark) blue background. Perhaps you can consider making the text white (with a text-shadow)
  • The tabs' behavior isn't very dynamic. You have to save the experiment before you see the new sorting order or a newly added language. Would it be an idea to add some javascript to show (at least) the newly added language? Having the sorting order or the current language code of the tab in question be dynamic seems of less importance to me.

@BeritJanssen
Copy link
Collaborator Author

I implemented the suggested style changes, and also went down deeper in the form to control the behaviour of BlockTranslatedContent: instead of being able to add these independently of the ExperimentTranslatedContent, these will now always be set and prefilled with values from the ExperimentTranslatedContent objects of the Experiment parent. I decided to leave any dynamic behaviour for later: let's see first if users find the form usable enough without it.

Changing the order dynamically would be, by my estimate, easier than creating an extra tab with a selected language on the fly, which requires dealing with several forms which do not correspond to a database object. Requiring the user to save first before seeing a new ExperimentTranslatedContent object reflected as a tab may then just be the lesser evil, rather than potentially creating many forms on the fly, some of which won't be valid if the user only selected a language, but entered no text for the required Name field. With the tab / hidden forms setup, it would be hard to find back which of the translated content forms throws the validation error.

theme_config = models.ForeignKey(ThemeConfig, on_delete=models.SET_NULL, blank=True, null=True)

def __str__(self):
content = self.get_fallback_content()
return content.name if content and content.name else self.slug
return self.slug
Copy link
Collaborator Author

@BeritJanssen BeritJanssen Dec 18, 2024

Choose a reason for hiding this comment

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

NB: This is now the behaviour of the Block string representation: instead of going through parent Experiment's translated content objects, it will just return the slug. Less fancy, but within the form, it's actually an advantage IMO if the reference to each Block object stays stable, even if the underlying language preferences change. Outside of the admin interface, the name property of Block can be used.

@@ -462,10 +457,6 @@ def get_fallback_content(self) -> "BlockTranslatedContent":
Returns:
Fallback content
"""

if not self.phase or self.phase.experiment:
return self.translated_contents.first()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't have blocks which aren't assigned to phases, and no phases which aren't assigned to experiments anymore. If that happens, I think it's better to throw an error.

@BeritJanssen BeritJanssen self-assigned this Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants