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

Modal loading indicator and close Modal event #1578

Merged
merged 15 commits into from
Aug 26, 2020
Merged

Modal loading indicator and close Modal event #1578

merged 15 commits into from
Aug 26, 2020

Conversation

azriel46d
Copy link
Contributor

Description

Changes proposed in this pull request:

  1. Added the loading indicator to the Modal. It utilises the same logic as per App file whereby it listens
    luigi.get-context to check for auto hide, luigi.hide-loading-indicator, luigi.show-loading-indicator to show and hide the loading respectively.
  2. Added an event luigi.close-modal which the Modal listens to.
  3. Added a closeModal function inside the ux client module which fires the even in point 2.

Related issue(s)

@CLAassistant
Copy link

CLAassistant commented Aug 20, 2020

CLA assistant check
All committers have signed the CLA.

@JohannesDoberer JohannesDoberer added the enhancement New feature or request label Aug 20, 2020
@JohannesDoberer JohannesDoberer added this to the Sprint 12 milestone Aug 20, 2020
@azriel46d
Copy link
Contributor Author

Travis is failing on
image
Is there anything that needs to be done from my end?

@ndricimrr ndricimrr self-assigned this Aug 21, 2020
@ndricimrr
Copy link
Contributor

Travis is failing on
image
Is there anything that needs to be done from my end?

@azriel46d For the moment no need to do anything about this as it should be handled in another ready-to-merge PR.

@maxmarkus maxmarkus self-assigned this Aug 24, 2020
Copy link
Contributor

@maxmarkus maxmarkus left a comment

Choose a reason for hiding this comment

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

Just a small suggestion and docu is not in sync. (lerna run docu should get automatically applied as pre-commit hook if you did a npm i in the luigi root folder initially).

And it would be great if you could adapt one of the e2e tests that open the modal, to also close it programmatically (could be as simple as calling win().LuigiClient.uxManager().closeModal() and expecting the modal to be closed afterwards.
You could take https://github.com/SAP/luigi/blob/master/test/e2e-test-application/e2e/tests/1-angular/microfrontends.spec.js#L27 and duplicate that one, already does everything except closing.

client/src/uxManager.js Outdated Show resolved Hide resolved
@azriel46d
Copy link
Contributor Author

Test has been added accordingly.

Copy link
Contributor

@ndricimrr ndricimrr left a comment

Choose a reason for hiding this comment

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

Just some small suggestions :)

Copy link
Contributor

@maxmarkus maxmarkus left a comment

Choose a reason for hiding this comment

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

Great Job! 👍

client/src/uxManager.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ndricimrr ndricimrr left a comment

Choose a reason for hiding this comment

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

suggestion to fix failing e2e-test:
In : luigi-client-link-manager-features.spec#L256

Change .find('button') to .contains('Click here')

@maxmarkus maxmarkus merged commit 286283c into SAP:master Aug 26, 2020
JohannesDoberer added a commit to JohannesDoberer/luigi that referenced this pull request Aug 31, 2020
…nfig

* master:
  Add up arrow key press to global search (SAP#1587)
  fix e2e test (SAP#1589)
  Update fiddle fd styles to 0.11.0 (SAP#1581)
  Modal loading indicator and close Modal event (SAP#1578)
  Integrate plugin readme in docu (SAP#1564)
  Luigi sample with Next.js (SAP#1579)
  Update version of FD styles to the Core 0.10.0 -> 0.11.0 (SAP#1569)
@azriel46d azriel46d deleted the feature/modal-loading-indicator branch September 3, 2020 10:35
@ndricimrr ndricimrr mentioned this pull request Sep 9, 2020
stanleychh pushed a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants