Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-44522

Top-level navigation does not show selected state

    XMLWordPrintable

Details

    • Blue Ocean 1.2-beta1, Blue Ocean 1.2-beta2

    Description

      The Blue Ocean interface does not display a selected state for the top-level navigation. This is especially problematic when plugins add additional top-level links to the interface. Because react renders so quickly it can create a usability problem where the user does not recognize that they have navigated to a new page because the links to do not transition to a "selected" state.

      The correct selected state is pictured in the attached screenshot (navigation-selected-state.png) and is already present in the CSS. In fact it's used for run result pages already. We just need to teach React about it for the top links.

       

       

      Attachments

        Issue Links

          Activity

            jlong John Long created issue -
            jlong John Long made changes -
            Field Original Value New Value
            Labels cloudbees-internal-steel
            jlong John Long made changes -
            Labels cloudbees-internal-steel blueocean cloudbees-internal-steel
            jamesdumay James Dumay made changes -
            Epic Link JENKINS-43955 [ 181487 ]
            jlong John Long made changes -
            Description The Blue Ocean interface does not display a selected state for the top-level navigation. This is especially problematic when plugins add additional top-level links to the interface. Because react renders so quickly it can create a usability problem where the user does not recognize that they have navigated to a new page because the links to do not transition to a different state.

            The correct selected state is pictured in the attached screenshot (navigation-selected-state.png) and is already present in the CSS. In fact it's used for [run result pages|https://ci.blueocean.io/blue/organizations/jenkins/admin%2Fdocker-gc/detail/docker-gc/10/pipeline] already. We just need to teach React about it for the top links.

             

             
            The Blue Ocean interface does not display a selected state for the top-level navigation. This is especially problematic when plugins add additional top-level links to the interface. Because react renders so quickly it can create a usability problem where the user does not recognize that they have navigated to a new page because the links to do not transition to a "selected" state.

            The correct selected state is pictured in the attached screenshot (navigation-selected-state.png) and is already present in the CSS. In fact it's used for [run result pages|https://ci.blueocean.io/blue/organizations/jenkins/admin%2Fdocker-gc/detail/docker-gc/10/pipeline] already. We just need to teach React about it for the top links.

             

             
            michaelneale Michael Neale made changes -
            Epic Link JENKINS-43955 [ 181487 ] JENKINS-35761 [ 171656 ]
            michaelneale Michael Neale made changes -
            Assignee Cliff Meyers [ cliffmeyers ]
            michaelneale Michael Neale added a comment -

            For cliff to investigate. I am not sure if this has aything to do with react, more just URI elements -> highlighted top element. 

            michaelneale Michael Neale added a comment - For cliff to investigate. I am not sure if this has aything to do with react, more just URI elements -> highlighted top element. 
            cliffmeyers Cliff Meyers added a comment - - edited

            cc sophistifunk on this one, maybe we can devise a solution here.

            Current mechanism for tab selection on Run Details is using TabLink, which internally uses router.isActive, and the route path matches exactly.

            route.isActive will also return true if the path matches a parent Route. For example, if current route path is '/organizations/jenkins/artifacts-150/activity':

            router.isActive('/') // returns true, as the this the top level route definition that matches everything
            router.isActive('/organizations/jenkins') // returns true, since we define a route with path="organizations/:organization"
            router.isActive('/organizations/jenkin') // returns false, since the organization path var doesn't match
            router.isActive('/organizations/jenkins/artifacts-150') // returns false, since that isn't actually a Route, but rather a Redirect that pushes the user to the /activity child route
            router.isActive('/organizations/jenkins/artifacts-150/activity') // returns true, exact match
            

            So technically this could be made to work if the top-level Link was a TabLink and its "to" prop pointed at a route path that was the parent for all other route. To make "Pipelines" do this would be harder (impossible?) since the route structure has a few different top-level routes (below "/" anyways) and using "/" would leave it active for all routes I think. sophistifunk does that sound right to you?

            If we can confirm the current pieces can't be made to work in a reasonable fashion, I may have an alternative idea or two.

            cliffmeyers Cliff Meyers added a comment - - edited cc sophistifunk on this one, maybe we can devise a solution here. Current mechanism for tab selection on Run Details is using TabLink, which internally uses router.isActive, and the route path matches exactly. route.isActive will also return true if the path matches a parent Route. For example, if current route path is '/organizations/jenkins/artifacts-150/activity': router.isActive( '/' ) // returns true , as the this the top level route definition that matches everything router.isActive( '/organizations/jenkins' ) // returns true , since we define a route with path= "organizations/:organization" router.isActive( '/organizations/jenkin' ) // returns false , since the organization path var doesn't match router.isActive( '/organizations/jenkins/artifacts-150' ) // returns false , since that isn't actually a Route, but rather a Redirect that pushes the user to the /activity child route router.isActive( '/organizations/jenkins/artifacts-150/activity' ) // returns true , exact match So technically this could be made to work if the top-level Link was a TabLink and its "to" prop pointed at a route path that was the parent for all other route. To make "Pipelines" do this would be harder (impossible?) since the route structure has a few different top-level routes (below "/" anyways) and using "/" would leave it active for all routes I think. sophistifunk does that sound right to you? If we can confirm the current pieces can't be made to work in a reasonable fashion, I may have an alternative idea or two.
            sophistifunk Josh McDonald added a comment -

            Yes, that makes sense. There's probably a way to query the router and get the link that `/` would redirect to, though? Then we could use that for the "Pipelines" tab url maybe?

            sophistifunk Josh McDonald added a comment - Yes, that makes sense. There's probably a way to query the router and get the link that `/` would redirect to, though? Then we could use that for the "Pipelines" tab url maybe?
            cliffmeyers Cliff Meyers added a comment -

            Yeah, I think we could make it work, although it might be a problem that crops up with other plugins that add a top-level link with their own customized route path structure. It feels a little kludgy to me perhaps? Or am I being too picky?

            I've been noodling on some other changes to routing that could possibly help out in this area. I've always thought that the top-level nav would work better if plugins were contributing simple pojos with a label and target URL, rather than a full-fledged React Link/TabLink component. (This is basically the "typed" extension point concept that Keith nearly has working). The extension point ideally would just iterate over the pojos and build up Links/TabLinks based on the data provided. An advantage to this might be that the typed "TopLink" could have a reference to the routes contributed by that plugin, so the "router.isActive" call could be driven off the full route list.

            Maybe this is over-engineering? It feels like relying on typed XP could be beneficial to formalize the contracts a bit, and bake logic into the XP itself rather than each plugin having to do little hacks to make things work the right way.

            WDYT?

            cliffmeyers Cliff Meyers added a comment - Yeah, I think we could make it work, although it might be a problem that crops up with other plugins that add a top-level link with their own customized route path structure. It feels a little kludgy to me perhaps? Or am I being too picky? I've been noodling on some other changes to routing that could possibly help out in this area. I've always thought that the top-level nav would work better if plugins were contributing simple pojos with a label and target URL, rather than a full-fledged React Link/TabLink component. (This is basically the "typed" extension point concept that Keith nearly has working). The extension point ideally would just iterate over the pojos and build up Links/TabLinks based on the data provided. An advantage to this might be that the typed "TopLink" could have a reference to the routes contributed by that plugin, so the "router.isActive" call could be driven off the full route list. Maybe this is over-engineering? It feels like relying on typed XP could be beneficial to formalize the contracts a bit, and bake logic into the XP itself rather than each plugin having to do little hacks to make things work the right way. WDYT?
            sophistifunk Josh McDonald added a comment -

            Agreed. Also worried that if we put in a bunch of kludgey stuff early, it'll get left in  That being said, we needn't take a bunch of steps right now to worry about what redirects may be done by future extensions, but rather just the current (and hardcoded IIRC) redirect that we have for '/'

            sophistifunk Josh McDonald added a comment - Agreed. Also worried that if we put in a bunch of kludgey stuff early, it'll get left in  That being said, we needn't take a bunch of steps right now to worry about what redirects may be done by future extensions, but rather just the current (and hardcoded IIRC) redirect that we have for '/'
            cliffmeyers Cliff Meyers added a comment -

            I fiddled with this for a while last week and doing this off URL-based matching is a bit of a mess. I just don't think it's the right way to do it. Rather we might want to consider a short-term solution along these lines:

            1. Plugin defines some kind of POJO which holds state as to whether it's active or not,
            2. The "Link" component added to the top level nav requires this component and reads its active state
            3. The route definition(s) from the plugin use the onChange handler to manipulate the state of that POJO as active or inactive.

            Later on, with typed extension points, this behavior could probably be baked in nearly 100%... but things will still get 'weird" if you have plugins that start to add routes that are nested underneath other routes from another plugin (say adding a tab to run details).

            Per jamesdumay I have some bigger fish to fry at the moment, although if I can crank out the above solution in hour or so perhaps I'll dive right in and do it.

            cliffmeyers Cliff Meyers added a comment - I fiddled with this for a while last week and doing this off URL-based matching is a bit of a mess. I just don't think it's the right way to do it. Rather we might want to consider a short-term solution along these lines: Plugin defines some kind of POJO which holds state as to whether it's active or not, The "Link" component added to the top level nav requires this component and reads its active state The route definition(s) from the plugin use the onChange handler to manipulate the state of that POJO as active or inactive. Later on, with typed extension points, this behavior could probably be baked in nearly 100%... but things will still get 'weird" if you have plugins that start to add routes that are nested underneath other routes from another plugin (say adding a tab to run details). Per jamesdumay I have some bigger fish to fry at the moment, although if I can crank out the above solution in hour or so perhaps I'll dive right in and do it.
            cliffmeyers Cliff Meyers made changes -
            Remote Link This issue links to "PR#1157 (Web Link)" [ 17144 ]
            cliffmeyers Cliff Meyers made changes -
            Sprint Blue Ocean 1.2-beta1 [ 336 ]
            cliffmeyers Cliff Meyers made changes -
            Status Open [ 1 ] In Progress [ 3 ]
            cliffmeyers Cliff Meyers made changes -
            Status In Progress [ 3 ] In Review [ 10005 ]
            cliffmeyers Cliff Meyers added a comment -

            jamesdumay I know you asked me to deprioritize this ticket, but I had an idea and was able to crank this out in about an hour... so I figured I'd throw the PR up and see what the thoughts are. If it gets thorny I'll just press pause on this thing for a while and fry some bigger fish. At least now if someone wants to tackle this bug they can perhaps make use of this code.

            cliffmeyers Cliff Meyers added a comment - jamesdumay I know you asked me to deprioritize this ticket, but I had an idea and was able to crank this out in about an hour... so I figured I'd throw the PR up and see what the thoughts are. If it gets thorny I'll just press pause on this thing for a while and fry some bigger fish. At least now if someone wants to tackle this bug they can perhaps make use of this code.
            jlong John Long added a comment -

            cliffmeyers can you link the pull request. Just curious about the approach.

            jlong John Long added a comment - cliffmeyers can you link the pull request. Just curious about the approach.
            cliffmeyers Cliff Meyers made changes -
            Remote Link This issue links to "PR#1157 (Web Link)" [ 17146 ]
            cliffmeyers Cliff Meyers made changes -
            Remote Link This issue links to "PR#1157 (Web Link)" [ 17146 ]
            cliffmeyers Cliff Meyers added a comment -

            jlong see link above.

            cliffmeyers Cliff Meyers added a comment - jlong see link above.
            jamesdumay James Dumay made changes -
            Sprint Blue Ocean 1.2-beta1 [ 336 ] Blue Ocean 1.2-beta1, Blue Ocean 1.2-beta2 [ 336, 341 ]
            cliffmeyers Cliff Meyers made changes -
            Resolution Fixed [ 1 ]
            Status In Review [ 10005 ] Resolved [ 5 ]
            jbriden Jenn Briden made changes -
            Status Resolved [ 5 ] Closed [ 6 ]

            People

              cliffmeyers Cliff Meyers
              jlong John Long
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: