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

Implement sidebars on react pages #1957

Closed

Conversation

marcustyphoon
Copy link

@marcustyphoon marcustyphoon commented Aug 12, 2020

This PR adds XKit.interface.react.sidebar functions that work on the new dashboard as identically as possible to XKit.interface.sidebar. It can be tested with this gist, #1966, #1967, and/or #1968.

This code currently places the XKit sidebar items below the main navigation area of tumblr.com/blog/my_blog_name, and above all other native Tumblr sidebar items, including "recommended blogs," "radar," and "sponsored."

The new add function is asynchronous and must be awaited before adding button handlers in extensions.

The added functions work on both react and non-react pages. removed

It also includes an XKit.interface.react.sidebar.add_sticky option removed

  • On react pages, this currently places the xkit sidebar items above all other native Tumblr sidebar items. resolved

marcustyphoon added a commit to marcustyphoon/XKit that referenced this pull request Aug 12, 2020
marcustyphoon added a commit to marcustyphoon/XKit that referenced this pull request Aug 14, 2020
@marcustyphoon marcustyphoon marked this pull request as draft August 22, 2020 19:24
@marcustyphoon marcustyphoon marked this pull request as ready for review August 22, 2020 20:34
Copy link

@hobinjk hobinjk left a comment

Choose a reason for hiding this comment

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

Overall code LGTM, some nitpicks inline

Extensions/xkit_patches.js Outdated Show resolved Hide resolved
* @param {Object[]} [section.items] - Array of objects containing button data
* @param {String} section.items[].id - Button element ID
* @param {String} section.items[].text - Visible button text
* @param {Number/String} [section.items[].count] - Text to be displayed as a counter on the button
Copy link

Choose a reason for hiding this comment

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

Another nitpick that doesn't require changing: Number|String is more commonly used. Technically in a type annotation like this it would be number|string since Number and String refer to the boxed versions of the primitive number and string types.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, all of the JSDoc—actually, most of the green code here—was copy-paste :D That did look odd though.

@marcustyphoon
Copy link
Author

This would (presumably?) need to be rewritten to take the vertical nav layout into account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants