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

Merged
merged 14 commits into from Sep 24, 2018

Conversation

Projects
None yet
2 participants
@milesdelliott
Contributor

milesdelliott commented Mar 7, 2018

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:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

Miles Elliott added some commits Mar 6, 2018

@aduth

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.

@milesdelliott

This comment has been minimized.

Show comment
Hide comment
@milesdelliott

milesdelliott Mar 22, 2018

Contributor

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.

Contributor

milesdelliott commented Mar 22, 2018

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.

@aduth

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.

Show outdated Hide outdated components/tab-panel/README.md Outdated
@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 13, 2018

Member

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

Member

aduth commented Sep 13, 2018

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

@milesdelliott milesdelliott changed the title from pass tab title to tab-panel child function to Pass tab object to tab-panel child function Sep 13, 2018

milesdelliott added some commits Sep 13, 2018

@milesdelliott

This comment has been minimized.

Show comment
Hide comment
@milesdelliott

milesdelliott Sep 13, 2018

Contributor

Sorry for the lack of activity on this, I've just updated the branch and ensured that all tests are passing

Contributor

milesdelliott commented Sep 13, 2018

Sorry for the lack of activity on this, I've just updated the branch and ensured that all tests are passing

@aduth aduth removed the [Status] Stale label Sep 18, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 18, 2018

Member

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?

Member

aduth commented Sep 18, 2018

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?

milesdelliott added some commits Sep 18, 2018

Allow passed object to be used as string
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.
@milesdelliott

This comment has been minimized.

Show comment
Hide comment
@milesdelliott

milesdelliott Sep 18, 2018

Contributor

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.

Contributor

milesdelliott commented Sep 18, 2018

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.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 18, 2018

Member

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.

My only concern would be the increased complexity and making it harder for other developers to understand,

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.

Member

aduth commented Sep 18, 2018

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.

My only concern would be the increased complexity and making it harder for other developers to understand,

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.

@aduth aduth referenced this pull request Sep 18, 2018

Merged

Allow double quotes and template string sometimes #7555

4 of 4 tasks complete

milesdelliott added some commits Sep 18, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 18, 2018

Member

Do we not use this component anywhere in Gutenberg?

Member

aduth commented Sep 18, 2018

Do we not use this component anywhere in Gutenberg?

@milesdelliott

This comment has been minimized.

Show comment
Hide comment
@milesdelliott

milesdelliott Sep 18, 2018

Contributor

I don't think so.

Contributor

milesdelliott commented Sep 18, 2018

I don't think so.

@aduth

aduth approved these changes Sep 18, 2018

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.

@milesdelliott

This comment has been minimized.

Show comment
Hide comment
@milesdelliott

milesdelliott Sep 19, 2018

Contributor

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?

Contributor

milesdelliott commented Sep 19, 2018

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?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 19, 2018

Member

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

Member

aduth commented Sep 19, 2018

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

milesdelliott added some commits Sep 19, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 24, 2018

Member

Looks great! Thanks!

Member

aduth commented Sep 24, 2018

Looks great! Thanks!

@aduth aduth merged commit be0d523 into WordPress:master Sep 24, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.22% but relative coverage increased by +50.95% compared to 02d530a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@milesdelliott

This comment has been minimized.

Show comment
Hide comment
@milesdelliott

milesdelliott Sep 24, 2018

Contributor

Thanks for your help!

Contributor

milesdelliott commented Sep 24, 2018

Thanks for your help!

@milesdelliott milesdelliott deleted the milesdelliott:fature/tab-pane/pass-tab-title-to-tab branch Sep 24, 2018

@aduth aduth added this to the 4.0 milestone Sep 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment