Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document Outline: Restructured buttons to anchors #7170

Closed

Conversation

anevins12
Copy link
Contributor

Description

Buttons inside the Document Outline component have been replaced with anchor tags in response to #7142.

How has this been tested?

  • The Document Outline component still performs the behaviour of moving focus to the block textarea. We are currently missing tests that check whether focus has indeed been moved, but this is an existing issue.
  • Tests pass from npm run test

Types of changes

  • Class name change;
  • Refactoring of variables;
  • Added hyperlink path functionality.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.

@anevins12
Copy link
Contributor Author

I'm not sure why tests are failing, when I run npm run test, all tests pass.

const titleNode = document.querySelector( '.editor-post-title__input' );
const titleId = () => {
if ( typeof titleNode.getAttribute( 'id' ) ) {
titleNode.setAttribute( 'id', 'id-' + titleNode.innerHTML );
Copy link
Member

@jorgefilipecosta jorgefilipecosta Jul 11, 2018

Choose a reason for hiding this comment

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

We are changing a dom node rendered by React directly, that may have undesirable and unexpected effects.
Is using the same events/focus mechanism we had but just change the button to an anchor an option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that would be great too. I think I tried to do that there, how would I change it to reflect a better practice?

@aduth
Copy link
Member

aduth commented Sep 13, 2018

Is it likely this will be resumed, or can it be closed?

@aduth aduth added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Sep 13, 2018
@gziolo
Copy link
Member

gziolo commented Oct 31, 2018

No update for a few weeks. I’m closing this one as it looks abandoned.

@gziolo gziolo closed this Oct 31, 2018
@aduth
Copy link
Member

aduth commented Oct 31, 2018

Related: #10815

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants