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

Make Connected Apps links available in Transifex #13011

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

drummer83
Copy link
Contributor

What? Why?

Following the discussion here on Slack I've made the links on the Connected Apps page available in Transifex so they can be edited without a code change.

What should we test?

  • Activate the feature toggle for Connected Apps.
  • Visit /admin/enterprises/Enterprise_Name/edit#/connected_apps_panel.
  • Test that the displayed description text, the displayed link and the link URL are still the same and working correctly.
  • Link should open in a new browser tab.

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

Copy link
Member

@dacook dacook 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 trying this out.

I think the intention was to have it as one general-purpose rich text editor so that you can customise it as needed. This is more flexible to cater for different use cases.

I see that transifex doesn't have the simple rich text editor that I hoped, but I was able to edit the existing URL which should be enough for @RachL's use case. Does this that solve the problem?

Screenshot 2024-12-02 at 4 11 32 PM
Screenshot 2024-12-02 at 4 12 16 PM

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

The current HTML block makes it easier to change the content and even add more than one link. So I wouldn't split it like in this pull request. I agree though that Transifex makes editing really difficult. Their intention is good: prevent people from breaking code. But it's really hard to change.

@mkllnk mkllnk closed this Dec 4, 2024
@drummer83
Copy link
Contributor Author

Maybe we could hear a comment from @openfoodfoundation/train-drivers-product-owners, like @RachL.

First of all, I think the functionality to see and even change HTML in Transifex is new. Not super new, but that's why we weren't aware of it.

Also, I think that having the HTML in Transifex is a completely new approach we haven't been using yet to such an extent - afaik. It may be tempting and promising to do so, but then it should be applied to many other parts of the app as well. But some things come to my mind:

  • a change of concept should be agreed on in the product or delivery circle,
  • not all our translators are HTML coders, and a lot can go wrong,
  • each iteration of finding and fixing mistakes requires a full week.

@dacook
Copy link
Member

dacook commented Dec 4, 2024

For this case, I think it's worth considering moving these descriptions out of translations, and into instance config (eg a new tab under /admin/general_settings). This would allow instance managers to configure for their context, and use a rich text editor.

I also have a suggestion for the current VINE use case, see mentioned PR. Is it possible to work on this as part of the funded feature?

@RachL
Copy link
Contributor

RachL commented Dec 9, 2024

Yes I agree with @drummer83 I don't think we can rely on changing the HTML. I will try to find budget for the settings ASAP and in the meantime maybe thanks for this PR @drummer83 !

@mkllnk mkllnk reopened this Dec 10, 2024
@mkllnk
Copy link
Member

mkllnk commented Dec 10, 2024

Okay. If changing the HTML is too hard then let's do the non-html version for now.

@rioug rioug added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Dec 10, 2024
@rioug rioug requested review from dacook and mkllnk December 10, 2024 23:06
config/locales/en.yml Show resolved Hide resolved
@RachL RachL added the pr-staged-au staging.openfoodnetwork.org.au label Jan 7, 2025
@RachL
Copy link
Contributor

RachL commented Jan 7, 2025

many thanks @drummer83 ❤️ LGTM! Merging :)

@RachL RachL merged commit 1147976 into openfoodfoundation:master Jan 7, 2025
55 checks passed
@RachL RachL removed the pr-staged-au staging.openfoodnetwork.org.au label Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants