Skip to content

fix(docs-infra): use .tooltip and highlight the currently active node in top-bar#33351

Closed
gkalpak wants to merge 2 commits intoangular:masterfrom
gkalpak:fix-aio-highlight-topbar
Closed

fix(docs-infra): use .tooltip and highlight the currently active node in top-bar#33351
gkalpak wants to merge 2 commits intoangular:masterfrom
gkalpak:fix-aio-highlight-topbar

Conversation

@gkalpak
Copy link
Copy Markdown
Member

@gkalpak gkalpak commented Oct 23, 2019

This PR switches to using NavigationNode#tooltip in aio-top-menu items and highlights the currently active node in top-bar. See individual commits for more details.

Related to #33239 and #33255.

@gkalpak gkalpak requested a review from a team October 23, 2019 12:11
@gkalpak gkalpak added comp: docs-infra state: blocked target: patch This PR is targeted for the next patch release type: bug/fix and removed cla: yes labels Oct 23, 2019
@googlebot
Copy link
Copy Markdown

☹️ Sorry, but only Googlers may change the label cla: yes.

@gkalpak
Copy link
Copy Markdown
Member Author

gkalpak commented Oct 23, 2019

The styles might need adjusting once #33255 has been merged. This PR should not be merged before that.

@mary-poppins
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except the styles really do need addressing (as pointed out in the PR description)

Screenshot 2019-10-24 at 09 53 58

The top-menu items have both a `title` and a `tooltip` property. The
`title` is used as text content, so there is little point in using it as
"tooltip" (via the HTMLElement's `title` property) too.

This commit switches to using the `tooltip` property to populate the
`title`. Note that in many cases, the `tooltip` property is derived from
`title` anyway (so there is no practical change in behavior in these
cases).
@gkalpak gkalpak force-pushed the fix-aio-highlight-topbar branch from 1e431f8 to 1787155 Compare February 20, 2020 13:06
@mary-poppins
Copy link
Copy Markdown

@gkalpak gkalpak force-pushed the fix-aio-highlight-topbar branch from 1787155 to 0ec3ce9 Compare February 20, 2020 13:35
@mary-poppins
Copy link
Copy Markdown

@gkalpak gkalpak added action: merge The PR is ready for merge by the caretaker action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed state: blocked action: merge The PR is ready for merge by the caretaker action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 20, 2020
@gkalpak gkalpak added the action: merge The PR is ready for merge by the caretaker label Feb 20, 2020
@gkalpak
Copy link
Copy Markdown
Member Author

gkalpak commented Feb 20, 2020

Rebased on master (now that #33255 has been merged) and marked for merging 🚀

mhevery pushed a commit that referenced this pull request Feb 20, 2020
The top-menu items have both a `title` and a `tooltip` property. The
`title` is used as text content, so there is little point in using it as
"tooltip" (via the HTMLElement's `title` property) too.

This commit switches to using the `tooltip` property to populate the
`title`. Note that in many cases, the `tooltip` property is derived from
`title` anyway (so there is no practical change in behavior in these
cases).

PR Close #33351
mhevery pushed a commit that referenced this pull request Feb 20, 2020
@mhevery mhevery closed this in 34b84f6 Feb 20, 2020
mhevery pushed a commit that referenced this pull request Feb 20, 2020
@gkalpak gkalpak deleted the fix-aio-highlight-topbar branch February 21, 2020 13:10
@angular-automatic-lock-bot
Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Mar 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release type: bug/fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants