Fixes duplicated title in header after edit #4760
Conversation
|
Thanks @nbianca for your pull request 👍. By analyzing the blame information on this pull request, I identified @eviltrout and @ZogStriP to be potential reviewers. |
|
You've signed the CLA, nbianca. Thank you! This pull request is ready for review. |
|
@nbianca really appreciate this, but I feel the fix is not in the right spot. I think it should always be safe to trigger the @eviltrout what do you thing? |
|
@SamSaffron By saying that it should be always safe to trigger
|
|
I see, My recommendation would be maybe to add an extra event called "header:update" that can be used whenever anyone feels like updating the header, then the logic can be centralized |
|
|
|
@nbianca keep in mind the logic is a bit more subtle than your test was. See: components/discourse-topic.js What I would like is for all the logic to be centralized there, discourse-topic already knows how to make the call here so it should be the one handling this new event. |
nbianca
force-pushed
the
fix_topic_title
branch
from
75feb33
to
0a73104
Compare
| @@ -117,16 +135,11 @@ export default Ember.Component.extend(AddArchetypeClass, Scrolling, { | |||
|
|
|||
| this.set('hasScrolled', offset > 0); | |||
|
|
|||
| // Trigger a header (topic title) update, but only if it is requried. | |||
| const topic = this.get('topic'); | |||
| const showTopic = this.showTopicInHeader(topic, offset); | |||
| if (showTopic !== this._lastShowTopic) { | |||
There was a problem hiding this comment.
I did this to avoid unnecessary rendering and event triggers.
| } | ||
|
|
||
| const offset = window.pageYOffset || $('html').scrollTop(); | ||
| this._lastShowTopic = this.showTopicInHeader(topic, offset); |
There was a problem hiding this comment.
Basically this forces an update.
There was a problem hiding this comment.
I also do not that think that offset should be a parameter and it would be better if it were moved inside showTopicInHeader.
There was a problem hiding this comment.
agree we can further refine this, but I think this fix for now is good
| this.appEvents.trigger('header:show-topic', topic); | ||
| } else { | ||
| this.appEvents.trigger('header:hide-topic'); | ||
| } |
There was a problem hiding this comment.
I think some of the duplication here is fine, cause otherwise you have to call showTopicInHeader twice, for no great gain
nbianca
force-pushed
the
fix_topic_title
branch
from
0a73104
to
8a506d9
Compare
nbianca
force-pushed
the
fix_topic_title
branch
from
8a506d9
to
f8a31d9
Compare
|
Thanks for the fix @nbianca ! |
Fixes the issue described here.