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

Improve control over luigi bootstrap process #552

Merged

Conversation

pekura
Copy link
Contributor

@pekura pekura commented May 24, 2019

No description provided.

@pekura pekura added enhancement New feature or request WIP Work in progress labels May 24, 2019
@pekura pekura added this to the Sprint_0 milestone May 24, 2019
@pekura pekura removed the WIP Work in progress label May 24, 2019
@pekura
Copy link
Contributor Author

pekura commented May 24, 2019

With Luigi.setConfig() the Luigi app can now tell Luigi that configuration has been changed. This leads to updating of:

  • navigation
  • context of the currently displayed MF view
  • header (title, logo)
  • left navigation footer
  • context switcher
  • product switcher
  • profile menu

If other parts of Luigi needs to be updateable in runtime we should create dedicated issues for it.

items: configProfileNav.items || []
};
this.set({ profileNav: profileNavData });
if (!this.get().navProfileListenerInstalled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Added this if block to avoid doOnStoreChange to be executed more than once.

@maxmarkus maxmarkus self-assigned this May 28, 2019
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 some minor questions

fn({
current: store.get(),
changed: { config: true },
previous: store.get()
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like current and previous is always the same, is this intentionally (for initial values?)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this is on purpose, we just want to trigger the function there.

core/src/navigation/ContextSwitcher.html Outdated Show resolved Hide resolved
@jesusreal jesusreal requested a review from maxmarkus May 31, 2019 12:50
…ver-luigi-bootstrap-process

# Conflicts:
#	core/src/core-api/config.js
Copy link
Contributor

@marynaKhromova marynaKhromova left a comment

Choose a reason for hiding this comment

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

Good job! 👍 I left only a couple of comments.

I've noticed as well a weird behavior when you cklick Add backdrop button.

Here are the steps to reproduce the issue:

  • go to Projects - > Project One
  • click Add backdrop button (nothing happens)
  • go to Keep Selected Example from the side navigation
  • you will see the overlay on the side navigation

overlay

And about documentation. Should it be done within this task or will we have the following task for it?

@jesusreal
Copy link
Contributor

Bug with backdrop modal is fixed. Documentation will be written in a follow up task. @marynaKhromova please check again.

@jesusreal jesusreal requested a review from marynaKhromova June 5, 2019 10:37
@marynaKhromova
Copy link
Contributor

Bug with backdrop modal is fixed. Documentation will be written in a follow up task. @marynaKhromova please check again.

Great, thank you 👍

Copy link
Contributor

@marynaKhromova marynaKhromova left a comment

Choose a reason for hiding this comment

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

LGTM!

@jesusreal jesusreal merged commit d632a17 into SAP:master Jun 5, 2019
@jesusreal jesusreal deleted the improve-control-over-luigi-bootstrap-process branch June 5, 2019 14:21
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.

4 participants