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

Fix dropdowns behavior on click events #301

Merged
merged 10 commits into from
Dec 27, 2018

Conversation

dariadomagala-sap
Copy link
Contributor

Description

Changes proposed in this pull request:

  • Fix dropdowns behavior on click events

Related issue(s)

@dariadomagala-sap dariadomagala-sap added bug Something isn't working area/luigi labels Dec 18, 2018
@dariadomagala-sap dariadomagala-sap added this to the Sprint_Swinka_6 milestone Dec 18, 2018
@pekura pekura self-assigned this Dec 18, 2018
@jesusreal jesusreal self-assigned this Dec 19, 2018
Copy link
Contributor

@jesusreal jesusreal left a comment

Choose a reason for hiding this comment

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

LGTM

@pekura
Copy link
Contributor

pekura commented Dec 20, 2018

It still does not work perfectly - in chrome when you click on a bottom part of the entry (somewhere in the last quarter of the highlighted area, below the text) it does not get activated - in the context chooser as well as the Logout button. In firefox the context chooser works fine but the Logout button has the same problem as in chrome.

@dariadomagala-sap dariadomagala-sap removed this from the Sprint_Swinka_6 milestone Dec 20, 2018
@dariadomagala-sap dariadomagala-sap added the WIP Work in progress label Dec 20, 2018
@dariadomagala-sap dariadomagala-sap removed the WIP Work in progress label Dec 21, 2018
Copy link
Contributor

@jesusreal jesusreal left a comment

Choose a reason for hiding this comment

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

It works well. I have a couple of refactoring suggenstions, please see my comments.

There are some refactorings I would definitely apply to the logic related to dropdowns, but this is out of scope for this task, we might do it in a different task. This is what I would change:

  • Context switcher should just read and not write the dropdownStates object and instead fire an event to topNav component, since it is not responsibility of context switcher to do that and we are duplicating some methods, more precisely closeAllDropdowns, toggleDropdownState and dropDownStatesNegated.
  • Have dropdown states as booleans and not as strings
  • Remove dropDownStatesNegated method
  • DropdownStates properties should be cased consistently with the rest of the code base (thisWay and not THAT_WAY)

this.set({
dropDownStates
});
},
closeDropdown(name) {
closeAllDropdowns() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would refactor this method, also in ContextSwitcher component, to the following:

closeAllDropdowns() {
  const dropDownStates = this.get().dropDownStates || {};
  Object.keys(dropDownStates).forEach(k => { dropDownStates[k] = 'false' });
  this.set({dropDownStates});
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dropDownStates is an empty object at the beginning. Adding the foreach loop without checking if the array of keys is not empty, would cause errors.


{#if !authorizationEnabled || isLoggedIn}
<ContextSwitcher {dropDownStates} bind:dropDownStates />
Copy link
Contributor

Choose a reason for hiding this comment

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

Not something you introduced, but {dropDownStates} is not needed here.

Copy link
Contributor

@jesusreal jesusreal left a comment

Choose a reason for hiding this comment

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

As it is working as required and you are working next week and I will be on vacation, I am approving it now. Please still consider my refactoring suggestions before merging.

@kwiatekus kwiatekus merged commit d611e57 into SAP:master Dec 27, 2018
@kwiatekus kwiatekus deleted the fix-dropdowns-actions branch December 27, 2018 12:25
stanleychh pushed a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021
* Fix dropdowns actions on click event
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants