Explore: Fix Browser title not updated on Navigation to Explore #34651
Conversation
- Update document.title on Explore page - Add unit test and snapshot for Wrapper component
axelavargas
added this to In Review (max. internal 8, external 3)
in Frontend Platform Backlog
via automation
axelavargas
requested review from
Elfo404,
hugohaggmark and
grafanabot
and removed request for
a team and
Elfo404
axelavargas
added
the
old backport v8.0.x
hugohaggmark
requested review from
dprokop,
aocenas and
ivanahuckova
and removed request for
grafanabot
torkelo
moved this from In Review (max. internal 8, external 3)
to In progress (max. internal 6)
in Frontend Platform Backlog
…approach - Remove unnecesary comments - Wrapper component is now inferring connected props - Update snapshot
Thanks, @hugohaggmark for the review, indeed it looks weird, I did the changes here 0e69909#diff-06aad0593d46c656f9c92b1a6e517f30e9758c1f1eff827194d81389ee5c74f6R35, I had to use directly the value from ps. I updated the gif in this PR with the new title |
Outstanding, will do another pass now. |
| @@ -1,18 +1,43 @@ | |||
| import React, { Component } from 'react'; | |||
| import { connect } from 'react-redux'; | |||
| import React, { PureComponent } from 'react'; | |||
There was a problem hiding this comment.
The change from Component to PureComponent should be fine AFAIK but maybe @ivanahuckova or @aocenas can confirm this too?
There was a problem hiding this comment.
As far as I know, the change should be okay. 🙂
There was a problem hiding this comment.
I don't think it should break anything but I have a feeling:
navModel: getNavModel(state.navIndex, 'explore'),
will break any benefit as it seems to create a new object every time.
There was a problem hiding this comment.
great 🙌🏾, thank you @ivanahuckova for taking a look, I will proceed to merge :)
There was a problem hiding this comment.
I don't think it should break anything but I have a feeling:
navModel: getNavModel(state.navIndex, 'explore'),will break any benefit as it seems to create a new object every time.
Hey, @aocenas thanks for checking out this PR 😄 , I was looking into getNavModel function, even though it generates a new object every time, the values are always the same if the parameters don't change, and because PureComponent only does a shallow comparison on the ComponentShouldUpdate I think will keep the benefits. I also verified the children's Wrapper component to confirm they are also PureComponents (to be sure these changers are not breaking anything).
let me know if I am mistaken(I still lacking context in the project 😅 ) , I will merge the PR but ofc, if we find later on this change don't add any benefits, we can put it back as before :)
There was a problem hiding this comment.
Thank you for working on this @axelavargas! 🧡 It has been in the backlog for quite some time and I am very happy that Explore has finally title.✨ LGTM!
- Update document.title on Explore page - Add unit test and snapshot for Wrapper component (cherry picked from commit cebe67a)
…ore (#34764) * Explore: Fix Browser title not updated on Navigation to Explore (#34651) - Update document.title on Explore page - Add unit test and snapshot for Wrapper component (cherry picked from commit cebe67a) * fix: remove snapshot for wrapper test Co-authored-by: Maria Alexandra <239999+axelavargas@users.noreply.github.com> Co-authored-by: Alexandra Vargas <alexa1866@gmail.com>
What this PR does / why we need it:
When navigating to Explore page the document.title was not being updated, this was caused because Explore page is not using
Pagecomponent that takes care of this functionality.##Notes about the Approach
I decided to add the functionality on Explore page by duplicating the same logic we applied on
Pagecomponent. That component is being used on other pages (Configuration, Alerting, etc...) but for the explore page will cause some unwanted styling issues and refactoring.Which issue(s) this PR fixes:
Fixes #32898
Special notes for your reviewer: