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: update captions to reflect the menus below [DHIS2-12655] #1977

Merged
merged 6 commits into from
Feb 21, 2024

Conversation

benguaraldi
Copy link
Contributor

This fixes the captions in the main menu and elsewhere in the Import/Export app so that they reflect the file formats that are found within.

Discussion of this between @cooper-joe and I can be found at:
https://dhis2.atlassian.net/browse/DHIS2-12655

I'm a little unsure of the best practice of changing text that is also found in the translation files, so I went ahead and altered them too. The English text will not match the other languages where it has been specified until this text is re-translated. Is there some notification that automatically happens with that?

@dhis2-bot
Copy link
Contributor

dhis2-bot commented Jan 29, 2024

🚀 Deployed on https://pr-1977--dhis2-import-export.netlify.app

@Philip-Larsen-Donnelly
Copy link
Contributor

Philip-Larsen-Donnelly commented Jan 29, 2024

@benguaraldi Unfortunately you can't change the .po files directly; they are pulled from the translation platform (transifex). In addition, I believe the source (en.pot) file is generated from the code. So, basically, the .pot and .po file changes should not be included; only the javascript need be updated.
(actually, the .pot should perhaps be included, but after generating it automatically)

@benguaraldi
Copy link
Contributor Author

@Philip-Larsen-Donnelly Thanks! That's a bummer that the changing of the text invalidates the close-but-not-exact translations.

This PR now has no changes to po files and the autogenerated pot file.

@tomzemp
Copy link
Member

tomzemp commented Feb 1, 2024

Thanks for the PR @benguaraldi 🙏

Looks good! Did you want to use the word format in the Data Import caption instead of files to be consistent with the others?
image

I would defer to @karolinelien or @cooper-joe on this one as I don't have strong feelings about what the language should be. As I now look at this, I do find it a bit confusing that our captions mention, for example, DXF 2 XML and ADX XML but then the options on the page will have XML and ADX. I guess we haven't been consistent before, however, and changing the options is probably going to be more confusing that changing the captions.

@tomzemp tomzemp requested review from karolinelien and removed request for tomzemp February 1, 2024 14:30
@benguaraldi
Copy link
Contributor Author

Good catch, @tomzemp! I've updated that text to match the others. Thanks!

@cooper-joe
Copy link
Member

Thanks for the PR @benguaraldi. The changes certainly make captions clearer and more accurate.

I would defer to @karolinelien or @cooper-joe on this one as I don't have strong feelings about what the language should be. As I now look at this, I do find it a bit confusing that our captions mention, for example, DXF 2 XML and ADX XML but then the options on the page will have XML and ADX. I guess we haven't been consistent before, however, and changing the options is probably going to be more confusing that changing the captions.

Yes, it seems the captions are now more accurate, but that accuracy now highlights the unclear labelling of the options. For example:

Data Export
Caption: Export metadata, such as data elements and organisation units, to JSON, CSV, DXF 2 XML, or ADX XML format.
Option labels: JSON, CSV, XML, ADX

So, in the above example it seems DXF 2 XML is being shortened to just XML in the options, and ADX XML is being shortened to just ADX in the options.

@benguaraldi, am I understanding this correctly? In this case, do you think the option names should be changed too? (to DXF 2 XML and ADX XML respectively).

@benguaraldi
Copy link
Contributor Author

benguaraldi commented Feb 9, 2024

@cooper-joe Thanks! Yes, that's right—I'm using ADX XML to mean the same thing as ADX in the options and DXF 2 XML to refer to the same thing as XML in the options, which is confusing.

Maybe the best thing is to change both DXF 2 and XML to DXF2 as well as ADX XML to ADX in both the options and the captions? That would remove XML from the captions and the options, but since we support two different types of XML, maybe that's appropriate?

(DXF2 seems clearer to me than DXF 2 and is how @larshelge styles it in this COP post, for instance.)

@bobjolliffe and @jimgrace definitely know more about these XML formats than I do, so might have ideas about how to best indicate them.

Another note—the TEI import and export also have an XML format. Is that also DXF2? Or is that a different kind of XML?

@benguaraldi
Copy link
Contributor Author

@cooper-joe Okay, I've matched the options and the captions and standardized on DXF2 and ADX, omitting the XML.

I did verify that the TEI import and export are also in the DXF2 format.

Perhaps this is now good to go?

@cooper-joe
Copy link
Member

I think this is a cleaner approach, yeah. My only concern is that, for less advanced users, they may be confused about getting .XML files output when they're choosing DXF2. You mentioned that:

That would remove XML from the captions and the options, but since we support two different types of XML, maybe that's appropriate?

I think what we could do, just to make this a smaller change, is use DXF2 (XML) and ADX (XML) as the labels for the options. A bit less clean, but if someone is looking for "I just want the option that gives me XML, like I've always done", this covers that use case.

Does that sound OK to you @benguaraldi?

@benguaraldi
Copy link
Contributor Author

@cooper-joe That's a great solution! Thanks!

I've implemented the XML parenthetical in the options only, not the captions, which is I think what you were looking for, but let me know if that's wrong.

Copy link
Member

@cooper-joe cooper-joe left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @benguaraldi, looks good to me.

@dhis2/platform-frontend, need a quick dev review on this one as it now touches the frontend (label changes only, though).

@cooper-joe cooper-joe requested a review from a team February 13, 2024 07:56
Copy link
Member

@tomzemp tomzemp left a comment

Choose a reason for hiding this comment

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

lgtm. @benguaraldi: do you want us to merge this for you?

@benguaraldi
Copy link
Contributor Author

@tomzemp Please! I'm not allowed to merge, apparently. :)

@tomzemp tomzemp merged commit a12296d into master Feb 21, 2024
10 checks passed
@tomzemp tomzemp deleted the DHIS2-12655-fix-action-captions branch February 21, 2024 15:43
dhis2-bot added a commit that referenced this pull request Feb 21, 2024
## [1.5.69](v1.5.68...v1.5.69) (2024-02-21)

### Bug Fixes

* update captions and options to reflect the menus below [DHIS2-12655] ([#1977](#1977)) ([a12296d](a12296d))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 1.5.69 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants