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

Multilingual/edit #142

Merged
merged 18 commits into from
Apr 4, 2024
Merged

Multilingual/edit #142

merged 18 commits into from
Apr 4, 2024

Conversation

lickem22
Copy link
Contributor

@lickem22 lickem22 commented Mar 31, 2024

Reviewer: @sidravi1

Estimate: 1h00


Ticket

Fixes: AAQ-428

Description, Motivation and Context

Goal

The goal of this PR is to add a feature to edit multilingual content.

Changes

What changed on the backend?

  • Added a route to edit content. The endpoint will edit an existing language version of a content and will also add a new language version to an existing text if the language version is not existing.
    -What changed on the frontend:
  • Updated the edit page to add new language version to a content.
  • Added a Modal to edit a new language version or go to landing page after succesful edit

How has this been tested?

Endpoint has been tested on the backend. And frontend has been tested as well to make sure it is working as intended. Test cases will be added later. No need to add a language as english is added as a default language by default in the migrations.

How to test

To test, add a few contents using the API (preferably in different languages) login to the frontend app and try updating. Both using the ContentBox and the read content modal. What to test?

  • Test that each language version is updated as it should.
  • Test that you can add a new language version using edit page.
  • Test that you redirected to the language chosen when viewing.

Checklist

Fill with x for completed. Delete any lines that are not relevant

  • My code follows the style guidelines of this project
  • I have reviewed my own code to ensure good quality
  • I have tested the functionality of my code to ensure it works as intended
  • I have resolved merge conflicts
  • I have updated the automated tests (if applicable)
  • I have updated the requirements (if applicable)
  • I have updated the README file (if applicable)
  • I have updated affected documentation (if applicable)

lickem22 and others added 10 commits March 26, 2024 19:36
* List content on frontend

* Add frontend changes

* Update retrieve language

* Delete language

* Fix conflicts

* fix linting

* format code and remove extra space above add button

* Improve language filter aesthetics and resizing behaviour

* Quick cleanup

---------

Co-authored-by: amir_emami <[email protected]>
@@ -259,5 +320,62 @@ const Header = ({ content_id }: { content_id: number | null }) => {
</Layout.FlexBox>
);
};
const SavedContentDialog = ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the modal to either redirect to the main page or to the edit page to add/edit another content

: languageOptions.map((language) => (
<MenuItem key={language.language_id} value={language.language_name}>{language.language_name}</MenuItem>
{loadingLanguages ? (
<MenuItem value="">Loading...</MenuItem>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This menu item is to add a new language version to an existing content. Handles what happens when we click on the plus button on the LanguageBar

@@ -10,6 +10,7 @@
},
"dependencies": {
"@emotion/react": "^11.11.3",
"@emotion/server": "^11.11.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add this package to fix the font bug.

Copy link
Member

Choose a reason for hiding this comment

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

So it does server side rendering / inlining of css? We should just remember that this is the first things we are doing server side. Everything else is client side!

@@ -0,0 +1,58 @@
import Document, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding this document is what fixed the font bug by making sure the Server Side Rendering is the same as the Client Side Rendering! It raised a warning in the console but I will fix it in next PR

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain how this works?

Copy link
Member

Choose a reason for hiding this comment

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

I don't seem to need this and can't recreate the view font bug.

Also, I'm ok accepting the font bug for now and we can find a solution later. This seem like a large decision we are making to switch to server side rendering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright. I will remove this fix and put the task in the backlog.

Copy link
Member

@sidravi1 sidravi1 left a comment

Choose a reason for hiding this comment

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

Thanks @lickem22! A few things to fix but ready for you to merge when done.

@@ -10,6 +10,7 @@
},
"dependencies": {
"@emotion/react": "^11.11.3",
"@emotion/server": "^11.11.0",
Copy link
Member

Choose a reason for hiding this comment

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

So it does server side rendering / inlining of css? We should just remember that this is the first things we are doing server side. Everything else is client side!

@@ -0,0 +1,58 @@
import Document, {
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain how this works?

@@ -0,0 +1,58 @@
import Document, {
Copy link
Member

Choose a reason for hiding this comment

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

I don't seem to need this and can't recreate the view font bug.

Also, I'm ok accepting the font bug for now and we can find a solution later. This seem like a large decision we are making to switch to server side rendering.

</ToggleButtonGroup>
{expandable && LANGUAGE_OPTIONS.length > langList.length && (
Copy link
Member

Choose a reason for hiding this comment

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

Any reason you took this type of login out? I like the idea that you don't need this circle if there are no languages left to add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add it back in next PR.

@@ -80,3 +77,7 @@ export const appStyles = {
flexGrow: 1,
},
};

export default function createEmotionCache() {
Copy link
Member

Choose a reason for hiding this comment

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

We can remove this if are not using emotion server?

@@ -121,8 +122,8 @@ async def save_content_to_db(
language=content_language,
content=content_db,
content_metadata=content.content_metadata,
created_datetime_utc=datetime.utcnow(),
updated_datetime_utc=datetime.utcnow(),
created_datetime_utc=datetime.now(tz.utc).replace(tzinfo=None),
Copy link
Member

Choose a reason for hiding this comment

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

At some stage we'll need to translate it to local time when showing it but save it as UTC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I understand. Why not directly get the UTC?

@lickem22 lickem22 merged commit 7fc968d into feature-multilingual Apr 4, 2024
1 check passed
@amiraliemami amiraliemami deleted the multilingual/edit branch April 24, 2024 19:58
@amiraliemami amiraliemami restored the multilingual/edit branch April 24, 2024 19:58
@amiraliemami amiraliemami deleted the multilingual/edit branch April 27, 2024 08:15
@amiraliemami amiraliemami restored the multilingual/edit branch April 27, 2024 08:16
@amiraliemami amiraliemami deleted the multilingual/edit branch April 27, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants