-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Pass tab object to tab-panel child function #5476
Pass tab object to tab-panel child function #5476
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be simpler to just pass the entire selectedTab
object, and the user could pick which properties they want to use? Particularly if they're already familiar with the object signature from the tabs
array, it seems like this would be simpler to use than predicting the order of the arguments.
Yeah it would be simpler and probably better long term. It would be a breaking change which is why I just added a new argument in this instance, but long term the entire object makes more sense, I can update the PR to pass the whole object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd personally be okay with it being a breaking change. Not likely to have seen much usage outside Gutenberg. To me, seems like we should have passed the whole object to begin with.
Are you able to resolve the currently failing tests? Let me know if you need help.
components/tab-panel/README.md
Outdated
@@ -3,7 +3,7 @@ TabPanel | |||
|
|||
TabPanel is a React component to render an ARIA-compliant TabPanel. It has two sections: a list of tabs, and the view to show when tabs are chosen. When the list of tabs gets focused, the active tab gets focus (the first tab if there isn't one already). Use the arrow keys to navigate between tabs AND select the newly focused tab at the same time. | |||
|
|||
TabPanel is a Function-as-Children component. The function takes `tabName` as an argument. | |||
TabPanel is a Function-as-Children component. The function takes `tabName` and `tabTitle` as arguments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs need to be updated taking into consideration we pass whole object.
Is it likely this will be resumed, or can it be closed? |
Sorry for the lack of activity on this, I've just updated the branch and ensured that all tests are passing |
Thanks for the updates @milesdelliott . Since April, my openness to breaking changes has shifted some, and I'm starting to question what we might be able to do to avoid breakage here. I still like the proposal as far as the desired interface (passing object as the argument), but wonder if there's a path for deprecation here. One thought I have is deprecating with a global warning message (example), keeping the argument value as a string for backwards-compatibility, but assigning properties into it so that a transition option exists. Example: const tab = new String( 'Tab name' );
tab.title = 'Tab name'; https://stackoverflow.com/a/5326882 It's very hacky, and we'd want to verify that using the constructed string this way works as we expect it to for existing usage. But I'd feel more comfortable about the transition if we could offer some compatibility. Thoughts? |
To avoid breaking changes, a String object is created from the tab name value, then the original properties of the tab object are assigned to this new String object. The passed object can now be used as if it were a string.
I'm all for avoiding breaking changes. I've updated the code to assign all the original properties of the tab object to a new String object with a value being the tab name. I tested it with a dummy component and it looks like it works well being used as the string or accessing the properties. My only concern would be the increased complexity and making it harder for other developers to understand, but I don't think it's too bad and it is a nice transition step to be completely taken out in the future. I kept the version for deprecation at 4.0.0, but let me know if that should be pushed out further. |
With 3.9.0 already in release candidate stage, we should consider all pending merges to be scheduled for 4.0.0, meaning 2 versions out would put it at 4.2.0 removal.
Definitely not great if it were a solution intended to be used long-term 😄 But since it's for deprecation compatibility, I think it's okay. |
@@ -63,6 +64,13 @@ class TabPanel extends Component { | |||
|
|||
const selectedTab = find( tabs, { name: selected } ); | |||
const selectedId = instanceId + '-' + selectedTab.name; | |||
|
|||
deprecated( 'Tab Panel child function argument used as string', { | |||
alternative: "Argument is now an object, access the name property directly.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the failing build might be related to the use of double quote here. We should use the single quote.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks!
Do we not use this component anywhere in Gutenberg? |
I don't think so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified the deprecation approach in an isolated environment:
https://codepen.io/aduth/pen/Ooawmr
An open question as to whether this is a component we should continue to offer if it's not used internally, though it seems generally useful.
Tests were failing, though it appeared to be an intermittent one. Hoping it works this time after a build restart.
Thanks for verifying. I think the test is failing because of the deprecation notice throwing a warning which the test doesn't like, but I'm guessing that's okay in this instance? |
You might look to do something like here to avoid the console log triggering the failure: https://github.com/WordPress/gutenberg/pull/9841/files#diff-7dd668718250393264047638da7e8c92L20 |
Looks great! Thanks! |
Thanks for your help! |
Description
Add a second argument to pass the Tab Title to the Tab Panel child function for situations where tab title may need to be highlighted further to indicate users which Tab they are on.
How Has This Been Tested?
I tested the initial change with a proof of concept implementation using the Tab Title, then I updated the Tab Panel tests to account for the change which all passed.
I am running Node version 8.9.1 and npm 5.5.1
Screenshots (jpeg or gifs if applicable):
Types of changes
This change adds an additional feature just by passing an additional argument to the function which is already required to be present. No breaking changes should occur, all code using the current code should continue to work fine
Checklist: