EZP-29319: I want to know that Confirmed items in UDW are clickable #71
Conversation
|
I'm not a big fan of unnecessary refactors. It will cause many conflicts when merging a fix up to master. |
|
@dew326 is right, let's split this up on two PRs |
|
@dew326 There were also 2 changes to JS needed. Not only to CSS. |
|
I think that partial refactoring is good; we should do this when touching some component. It would be rather a bad idea to change e.g. |
|
@tischsoic at first, you should focus only on fixing the issue making as little changes as possible. Then in the separate PR, you might do some code refactoring and there we'll discuss your new approach. Is it ok for you? |
|
@sunpietro I will just remove unnecessary refactoring. |
tischsoic
force-pushed
the
EZP-29319-i-want-to-know-that-confirmed-items-in-udw-are-clickable
branch
2 times, most recently
from
ec33f05
to
5fe4ebc
Compare
| const titles = items.map((item) => item.ContentInfo.Content.Name).join(', '); | ||
| const anyItemSelected = !!items.length; | ||
| const cssClassOnAnyItemSelected = anyItemSelected ? 'c-selected-content--any-item-selected' : ''; | ||
| const cssClasses = `c-selected-content ${cssClassOnAnyItemSelected}`; |
There was a problem hiding this comment.
I would be better to use something like this: https://www.npmjs.com/package/classnames
What do you think, @sunpietro @dew326 ?
tischsoic
force-pushed
the
EZP-29319-i-want-to-know-that-confirmed-items-in-udw-are-clickable
branch
from
5fe4ebc
to
210d9e4
Compare
tischsoic
force-pushed
the
EZP-29319-i-want-to-know-that-confirmed-items-in-udw-are-clickable
branch
from
210d9e4
to
1b75fc3
Compare
| * @memberof PopupComponent | ||
| */ | ||
| hidePopup() { | ||
| hidePopup(event) { | ||
| event.stopPropagation(); |
There was a problem hiding this comment.
What was the reason to stop event propagation?
There was a problem hiding this comment.
Now there is no exact reason for this, but there was during development.
Popup is rendered within SelectedContentComponent.
So there is that chain of events:
- you click on popup.
hidePopuphides popup, and callsonClosefromSelectedContentComponent- click event reaches top div of
SelectedContentComponent
What is important is that in some point of developement there was togglePopup as onClick on the top div of SelectedContentComponent.
So back then event from point 3 was reaching top div, and in turn togglePopup was called so popup was reopened again. Because of togglePopup being called.
event reaches popup -> hidePopup -> event reaches top div -> togglePopup
hide + toggle === no change
But at the end I moved onClick to another div and there is no longer need for stopPropagation .
Remove this stopPropagation?
There was a problem hiding this comment.
Yes, now it's not needed here.
| {this.renderLimitLabel()} | ||
| <div className="c-selected-content__content-names" onClick={this.togglePopup.bind(this)}> | ||
| {titles.length ? titles : this.props.labels.selectedContent.noConfirmedContent} | ||
| <div className={infoCssClasses} onClick={this.togglePopup.bind(this)}> |
There was a problem hiding this comment.
Binding should go to constructor
…pin hidePopup method
…zsystems#71) * EZP-29319: Fix Confirmed items in UDW - improve clickability. * EZP-29319: Move bind to constructor; remove stopPropagation from popupin hidePopup method
https://jira.ez.no/browse/EZP-29319