Bug/jenkins 43948 step header disappears clicked #1070
Conversation
… expanding after first click
…rwise when URL value changes the child result item will undergo remount and replay animations
|
@scherler would just like some feedback from you that these changes are appropriate. See my comments inline. Thanks!! |
| if (this.pager.pending) { | ||
| logger.debug('pending returning null'); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This was causing the entire ResultItem - header included - to disappear while data was loading. Removing this didn't seem to cause any issues and ATH passed.
There was a problem hiding this comment.
ah yes, react accepts null as meaning "remove this component or don't bother rendering". Looks like left over debug code.
| @@ -139,7 +134,7 @@ export class Step extends Component { | |||
| liveFormat={t('common.date.duration.format', { defaultValue: 'm[ minutes] s[ seconds]' })} | |||
| hintFormat={t('common.date.duration.hint.format', { defaultValue: 'M [month], d [days], h[h], m[m], s[s]' })} | |||
| />); | |||
| return (<div className={logConsoleClass} key={this.pager.currentLogUrl}> | |||
| return (<div className={logConsoleClass}> | |||
There was a problem hiding this comment.
After making the above change, I noticed that when the log data loaded, the slide/open animation would play again. It wasn't terrible but it looked a little funny. I noticed that the value of this.pager.currentLogUrl was undefined before the data fetch, and then populated after. This caused the component to be created/redestroyed, so the ResultItem was playing its animation again. This fix seems appropriate but I am curious if there is a specific reason you set a key on this component. It seems unnecessary since the parent Steps component is already defining a key on each Step?
There was a problem hiding this comment.
@cliffmeyers I am not 100% sure but I think that has been there because of the effect it has on rendering, I remember adding it when using the satges and steps and the steps had been from the old stage, but I guess that is not needed anymore then ;)
|
|
|
@cliffmeyers nice one 👍 |
|
🐝 nice one cliff on this critical issue! |
| @@ -139,7 +134,7 @@ export class Step extends Component { | |||
| liveFormat={t('common.date.duration.format', { defaultValue: 'm[ minutes] s[ seconds]' })} | |||
| hintFormat={t('common.date.duration.hint.format', { defaultValue: 'M [month], d [days], h[h], m[m], s[s]' })} | |||
| />); | |||
| return (<div className={logConsoleClass} key={this.pager.currentLogUrl}> | |||
| return (<div className={logConsoleClass}> | |||
There was a problem hiding this comment.
@cliffmeyers I am not 100% sure but I think that has been there because of the effect it has on rendering, I remember adding it when using the satges and steps and the steps had been from the old stage, but I guess that is not needed anymore then ;)
Description
Submitter checklist
Reviewer checklist