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.

Sign up for GitHub

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

Jump to bottom

Confirmation modal enhancements #1900

Merged
merged 12 commits into from
Feb 26, 2021
Merged

Confirmation modal enhancements #1900

merged 12 commits into from
Feb 26, 2021

Conversation

stanleychh
Copy link
Contributor

  • Apply FIORI 3 Title styling
  • Make confirmation modal's body supports HTML syntax (allow simple text formatting options, e.g. bold, italic, etc)

Related issue(s)
Fixes #1896

@stanleychh stanleychh added this to the Sprint 16 milestone Feb 18, 2021
@stanleychh stanleychh added the enhancement New feature or request label Feb 18, 2021
hardl
hardl previously requested changes Feb 18, 2021
View reviewed changes
core/src/ConfirmationModal.html
@@ -14,7 +14,7 @@ <h3 class="fd-title fd-title--h5">{settings.header}</h3>
</div>
</div>
</header>
<div class="fd-message-box__body">{settings.body}</div>
<div class="fd-message-box__body">{@html settings.body}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

i can't find any sanity check for settings.body, so this allows to inject anything into core app

core/src/ConfirmationModal.html
@@ -14,7 +14,7 @@ <h3 class="fd-title fd-title--h5">{settings.header}</h3>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

in the line above, h3 is used, but fd styles has h2
image

@stanleychh
Copy link
Contributor Author

@hardl , I have pushed an improvement which will sanitize HTML body and also restore those formatting tags. Please review it again. Thanks

@stanleychh stanleychh requested a review from hardl February 22, 2021 13:09
legteodav
legteodav reviewed Feb 23, 2021
View reviewed changes
core/src/ConfirmationModal.html Outdated
Comment on lines 79 to 86
const sanitizedHtml = EscapingHelpers.sanitizeHtml(settings.body);
settings = {
...settings,
body: EscapingHelpers.restoreSanitizedTags(
sanitizedHtml,
restoreSanitizedTags
)
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not be better to just create a new method in EscapingHelpers.sanatizeHtmlWithSpecialChar (or a better name) where you can inject the whole logic, instead of putting it in ConfirmationModal.html?
I guess that also the restoreSanitizedTags Array should be moved to EscapingHelpers.js (I think we could reuse it in other element, like Alert or whatever... )

@legteodav legteodav self-assigned this Feb 23, 2021
@stanleychh
Copy link
Contributor Author

Thanks @legteodav . I have introduced a new sanatizeHtmlExceptTextFormatting function to handle those logics.

@JohannesDoberer JohannesDoberer self-assigned this Feb 23, 2021
@JohannesDoberer
Copy link
Contributor

JohannesDoberer commented Feb 25, 2021 •
edited
Loading

We should document which elements are allowed. Fine as a follow-up task as well

alexandra-simeonova
alexandra-simeonova reviewed Feb 26, 2021
View reviewed changes
docs/luigi-client-api.md Outdated Show resolved Hide resolved
docs/luigi-core-api.md Outdated Show resolved Hide resolved
@stanleychh @alexandra-simeonova
Update docs/luigi-client-api.md
81d690e 
Co-authored-by: Aleksandra Simeonova <[email protected]>
stanleychh and others added 2 commits February 26, 2021 10:52
@stanleychh stanleychh dismissed hardl’s stale review February 26, 2021 10:27

Already reviewed

@stanleychh stanleychh merged commit 4b19ecb into SAP:master Feb 26, 2021
@stanleychh stanleychh deleted the 1896-confirmation-modal-enhancements branch February 26, 2021 10:32
JohannesDoberer added a commit to JohannesDoberer/luigi that referenced this pull request Mar 1, 2021
@JohannesDoberer
Merge branch 'master' into user-settings-test-improvents
d92d2f2 
* master:
  Usersettings refinements (SAP#1906)
  Update luigi-auto-routing.service.ts
  Confirmation modal enhancements (SAP#1900)

# Conflicts:
#	test/e2e-test-application/e2e/tests/1-angular/user_settings_dialog.spec.js
#	test/e2e-test-application/src/luigi-config/extended/userSettings.js
@JohannesDoberer JohannesDoberer mentioned this pull request Mar 1, 2021
Merged
JohannesDoberer added a commit to JohannesDoberer/luigi that referenced this pull request Mar 1, 2021
@JohannesDoberer
Merge branch 'update-fiddle-to-v1.10' of github.com:JohannesDoberer/l…
58be689 
…uigi into update-fiddle-to-v1.10

* 'update-fiddle-to-v1.10' of github.com:JohannesDoberer/luigi:
  Confirmation modal enhancements (SAP#1900)
  Blog post with Luigi updates (SAP#1880)
  Fix error in navigation docs
  1824 Silent Renew for OIDC documentation
  theming.nodeViewURLDecorator should also be applied to settings micro frontends viewUrl
  General/User settings docu review
stanleychh added a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021
@stanleychh
Confirmation modal enhancements (SAP#1900)
f320ee1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legteodav legteodav legteodav approved these changes

@JohannesDoberer JohannesDoberer JohannesDoberer approved these changes

@alexandra-simeonova alexandra-simeonova alexandra-simeonova approved these changes

@maxmarkus maxmarkus Awaiting requested review from maxmarkus

@ndricimrr ndricimrr Awaiting requested review from ndricimrr ndricimrr is a code owner

@UlianaMunich UlianaMunich Awaiting requested review from UlianaMunich

@hardl hardl Awaiting requested review from hardl hardl is a code owner

Assignees

@legteodav legteodav

@JohannesDoberer JohannesDoberer

Labels
enhancement New feature or request
Projects
None yet
Milestone
Sprint 16
Development

Successfully merging this pull request may close these issues.

confirmation modal enhancements
5 participants
@stanleychh @JohannesDoberer @legteodav @hardl @alexandra-simeonova