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

feat(DHIS2-17002): configure text to display before and after section #2812

Merged
merged 2 commits into from
Mar 18, 2024

Conversation

kabaros
Copy link
Contributor

@kabaros kabaros commented Mar 6, 2024

This PR adds the ability to configure custom HTML text before and after a section. This implements DHIS2-17002

Related data-entry PR: dhis2/aggregate-data-entry-app#373

text_before_after_section.webm

Copy link
Member

@Birkbjo Birkbjo left a comment

Choose a reason for hiding this comment

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

Minor comment, but looks good to me!

src/EditModel/SectionDialog.component.js Outdated Show resolved Hide resolved
@kabaros kabaros force-pushed the DHIS2-17002/after-before-section-text branch 2 times, most recently from abfcdce to 6771932 Compare March 14, 2024 13:49
@kabaros kabaros requested a review from Birkbjo March 14, 2024 13:50
@kabaros
Copy link
Contributor Author

kabaros commented Mar 14, 2024

@Birkbjo I made this change based on Michael comment about cleaning the HTML on entry - let me know if it looks ok

@kabaros kabaros force-pushed the DHIS2-17002/after-before-section-text branch from 6771932 to 66a2504 Compare March 14, 2024 15:21
@@ -359,6 +361,17 @@ class SectionDialog extends React.Component {
});
};

handleSectionTextUpdate = (fieldName) => (ev, text) => {
const cleanHtml = DOMPurify.sanitize(text);
Copy link
Member

@Birkbjo Birkbjo Mar 14, 2024

Choose a reason for hiding this comment

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

Should we call this in componentWillReceiveProps as well? So that the initial-value is sanitized as well?

Of course this should ideally be done server-side, but this could help if someone opened a section that has been changed through API or some other means (and don't change the text).

Maybe we should extract it to it's own method?

sanitizeDisplayOptions(displayOptions) //could be static or outside, idk

// in this code:

const newDisplayOption = sanitizeDisplayOptions({ displayOptions: { ...this.state.displayOptions, [fieldName]: cleanHtml }})

// componentWillReceiveProps

  displayOptions: props.sectionModel.displayOptions
      ? this.sanitizeDisplayOptions(JSON.parse(props.sectionModel.displayOptions))
       : {},

If you think it's unnecessary to always sanitize both keys, I'm fine with also sanitizing them directly.

sanitizeText(text)

const newDisplayOptions = {
...this.state.displayOptions,
[fieldName]: sanitize(text)
}

// would make componentWillReceiveProps handling a bit more convoluted though

Just a bit more convenient to pass the entire thing.

Copy link
Member

Choose a reason for hiding this comment

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

Disregard my last comment. I think it would be better to just run sanitization in saveSection, right?

@@ -359,6 +374,15 @@ class SectionDialog extends React.Component {
});
};

handleSectionTextUpdate = (fieldName) => (ev, text) => {
this.setState({
Copy link
Member

Choose a reason for hiding this comment

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

To be pedantic this should use the callback-style for prevValue, right?
Shouldn't really matter, though...

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 don't think this is necessary anymore, just a nice to have - from the docs:

You don’t have to do this, but it’s handy if you want to update state multiple times during the same event.

@kabaros kabaros force-pushed the DHIS2-17002/after-before-section-text branch from f22a0a1 to f9abe6b Compare March 18, 2024 10:20
@kabaros kabaros force-pushed the DHIS2-17002/after-before-section-text branch from f9abe6b to f277164 Compare March 18, 2024 10:25
@kabaros kabaros merged commit 94ba5d3 into master Mar 18, 2024
5 checks passed
@kabaros kabaros deleted the DHIS2-17002/after-before-section-text branch March 18, 2024 10:42
dhis2-bot added a commit that referenced this pull request Mar 18, 2024
# [32.27.0](v32.26.2...v32.27.0) (2024-03-18)

### Features

* **DHIS2-17002:** configure text to display before and after section ([#2812](#2812)) ([94ba5d3](94ba5d3))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 32.27.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Birkbjo pushed a commit that referenced this pull request Apr 9, 2024
…#2812)

* feat(DHIS2-17002): configure text to display before and after section

* refactor: apply code review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants