3.0.0 Button.js #632

onluiz · 2018-08-13T23:41:21Z

Description

It closes #583 (I hope 😄 )

This PR updates Button component to work with materializecss 1.0.0-rc.2. It includes:

New class .btn-small
Now if your element has a class like .btn-large or .btn-small you no longer need the .btn class
A update of Button component docs (used materializecss new Button docs page as example)
A page for documenting Floating Action Button component (used materializecss new Button docs page as example)
A helper file called string-helper. It is used to captalize and add spaces to titles (for the navbar and header of the page). It was necessary since the Floating Action Component docs page had its title rendered as "FloatingActionPage" (in the header of the page and in the navbar)
A fix for Floating Action Button. Now it is initialized when fab prop is passed
A test for new class .btn-small

There is some items I would like the opinion from the others contributors:

I have used materializecss docs as base for those changes in our docs. Was it ok?
What you think about my solution for nav items with more than one word (Floating Action Button)? Any sugestion of how it could be better?
The fix for Floating Action Button was valid? Is it ok to initialize the component the way I have done?
There is any other unit tests you guys want me to add?

@petergarnaes, sorry but could you review this PR again? I have created a new clean branch. And thanks for helping me.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Manual tests using the docs
Unit tests

Checklist:

I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
I have not generated a new package version. (the maintainers will handle that)

…utton

petergarnaes · 2018-08-14T07:16:42Z

docs/src/pages/ButtonsPage.js

@@ -35,28 +32,40 @@ const ButtonsPage = () => (
        </ReactPlayground>
      </Col>
      <h4 className='col s12'>


What about an example of a Flat button, corresponding to the one in materializecss?

petergarnaes · 2018-08-14T07:19:16Z

docs/src/pages/Samples.js

@@ -50,6 +50,7 @@ export default {
  sideNav: require('!raw-loader!../../../examples/SideNavBasic.js'),
  sliders: require('!raw-loader!../../../examples/Sliders.js'),
  submitButton: require('!raw-loader!../../../examples/SubmitButton.js'),
+  smallButtom: require('!raw-loader!../../../examples/SmallButtom.js'),


Small typo in file name 😅

petergarnaes · 2018-08-14T07:22:25Z

src/Button.js

    if (
      typeof $ !== 'undefined' &&
      (typeof tooltip !== 'undefined' || typeof tooltipOptions !== 'undefined')
    ) {
      $(this._btnEl).tooltip(tooltipOptions);
    }
+    if (fab && this._floatingActionBtn) {
+      $(this._floatingActionBtn).floatingActionButton();


You cannot use jQuery, you should do something like: M.FloatingActionButton.init(elems, options);. Same goes for the use of jQuery right above on line 21.

Also remember to destroy on componentDidUnmount, on both destroy and re-initialize on componentDidUpdate to make sure new props are used as options.

@alextrastero, what you think of it? componentDidUnmount actualy doesn't exist in react life cicle (at least a coudn't find it in the docs), but there is componentWillUnmount.

In that case, we would have something like:

componentDidMount -> M.FloatingActionButton.init

componentWillUnmount - > M.FloatingActionButton.destroy

In that situation, we will only have do save the instance returned in M.FloatingActionButton.init and then destroy it on componentWillUnmount.

Does it makes sense for you guys? Any tought about best ways to implement it?

@alextrastero @petergarnaes

You are absolutely right, componentWillUnmount is what I meant. The way you describe is exactly whas is done elsewhere.

Also remember componentDidUpdate. In my PR I destroy, and re-initialize, don't think there is any other way to give the instance new options.

petergarnaes

Great start, you still need to remove any dependency on jQuery. Also you will need to test the new way of initializing and destroying the instance, see https://github.com/react-materialize/react-materialize/blob/3.0.0-beta.0/test/Collapsible.spec.js#L8-L44 for an example.

alextrastero · 2018-08-14T08:24:28Z

Thanks @petergarnaes for the review :) and thanks @onluiz for this massive change well done

onluiz · 2018-08-14T15:38:22Z

@alextrastero @petergarnaes , thanks for the feedbacks. I'll fix it all as soon as possible.

I fixed the easy ones for now.

alextrastero

Please don't commit the package-lock.js file :)

alextrastero · 2018-08-15T07:53:25Z

examples/FixedActionButton.js

@@ -2,7 +2,7 @@ import React from 'react';
 import Button from '../src/Button';

 export default
-  <Button floating fab='vertical' faicon='fa fa-plus' className='red' large style={{bottom: '45px', right: '24px'}}>
+<Button floating fab='vertical' faicon='fa fa-plus' className='red' large style={{bottom: '45px', right: '24px'}}>


what does this faicon='fa fa-plus' prop do?

I think you can remove it :)

onluiz · 2018-08-15T16:25:57Z

@alextrastero, later I will fix Button.spec and add new tests.

petergarnaes · 2018-08-16T07:54:56Z

Nice work! However, you still need the componentDidUpdate.

Maybe we should split the Button and FloatingActionButton into two separate components? This mirrors the new materialize-css better, since in v0.100.2 it was all buttons with no javacript, but in v1.0.0-rc.2 it is now two separate pages, where FAB requires javascript and init. This would also allow you to pass options to the init call of FAB, which is missing right now (direction,hoverEnabled,toolbarEnabled). For an example of this see: https://github.com/react-materialize/react-materialize/blob/master/src/Carousel.js#L96

Hope it makes sense 😅

onluiz · 2018-08-16T15:56:14Z

@petergarnaes nice insights. I will try it out and shows you the results.

EDIT: I will mimic the Floating Action Button from materializecss: https://materializecss.com/floating-action-button.html

onluiz · 2018-08-16T16:14:35Z

I will be closing this PR for now. When I have all the review requests done I will open it again.

Thanks guys!

alextrastero · 2018-08-17T08:30:59Z

Why close?

alextrastero · 2018-08-17T08:32:31Z

src/Button.js

+    if (!M) return;
+
+    const { tooltip, tooltipOptions, fab } = this.props;
+    if (tooltip || tooltipOptions) {


If tooltipOptions is defined but there is no actual tooltip text should it still appear?

You're right :)

petergarnaes · 2018-08-31T08:30:00Z

Awesome! Is it possible to test the tooltip initialization also?

onluiz · 2018-08-31T12:25:01Z

@petergarnaes , yes! :)

In Button.spec there is tests for tooltip (init and destroy) and for fab (init and destroy).

=)

petergarnaes · 2018-08-31T13:21:08Z

Somehow missed it 😵 Great job!

alextrastero

Hey @onluiz, nearly there, just a few comments:

Please merge the docs of buttons and FloatingActionButtonsPage, I would rather keep the pages on the docs unified.

Did you delete package-lock? Seems like -10,106 changes is quite a lot :)

alextrastero · 2018-09-02T10:07:11Z

src/Button.js

+    if (fab && this._floatingActionBtn) {
+      this.instance = M.FloatingActionButton.init(this._floatingActionBtn);
+    }
+  }


I don't think you need to check for this._floatingActionBtn on L23

The -10,106 changes are actually from my commit that removes the package.lock. BUT, I may have done something wrong trying to remove the file. @alextrastero, could you help me on this? What is the proper way to remove the file? Sorry about the trouble.

No problem! You can checkout any file from any branch like so:
git checkout master package-lock.json

Omg it worked 😄

Now I will work on the others tasks:

Remove check this._floatingActionBtn on L23

Merge the docs of buttons and FloatingActionButtonsPage

:)

Done :)
@alextrastero

alextrastero · 2018-09-21T09:26:55Z

@onluiz ping

onluiz · 2018-09-24T12:22:25Z

@alextrastero , sorry. I have been dealing with health problems and couldn't finish my PRhe. Can I finish it in the next days? 😸

alextrastero · 2018-09-25T08:32:05Z

Of course buddy! no worries, get well!

alextrastero · 2018-11-01T08:59:09Z

Nice! one last thing are we are done! checkout the modal snapshot from the beta branch since we are not changing that file on this PR :)

onluiz · 2018-11-01T12:33:27Z

@alextrastero all right!

I'll sync my fork so a can let it all working.

onluiz · 2018-12-14T11:22:18Z

@alextrastero , really sorry for the delay =(

I tried to revert the snapshot but the tests began to fail.

I'll bring more details here about the problem. And again, sorry for the delay.

alextrastero · 2018-12-14T13:32:59Z

what if you run npm test -- -u and then commit the snapshots, wouldn't that be enough?

onluiz · 2018-12-14T15:14:17Z

@alextrastero think it worked =)
What you think?

This reverts commit 696f65e.

alextrastero · 2018-12-14T16:24:04Z

Great job @onluiz, thanks for the contribution. As as a next step would you like to to create a storybook story for the buttons? Have a look in here: https://github.com/react-materialize/react-materialize/tree/3.0.0-beta.0/stories

onluiz · 2018-12-14T18:28:57Z

@alextrastero I would love to! I am having such a great learning time here 😄
Thanks for your patience 😆

onluiz added 7 commits August 11, 2018 22:12

Merge https://github.com/onluiz/react-materialize into 3.0.0-beta.0-B…

0d6178d

…utton

Add new examples for Buttom component from new materializecss version

b8ca981

Update Floating Action Button docs

a26904d

Remove Floating Action Buttom from Buttom Docs

853d6cd

Remove unused imports from ButtonsPage and Floating Action Buttons Page

940627f

Add test for button with small styles applied

ee27853

Update Button from new materializecss version

9f14daf

onluiz changed the title ~~Update Button from new materializecss version~~ 3.0.0 Button.js Aug 13, 2018

petergarnaes reviewed Aug 14, 2018

View reviewed changes

petergarnaes suggested changes Aug 14, 2018

View reviewed changes

Fix typo in smallButton sample

12f62b5

Fix typo and merge changes

ee6b9d7

onluiz closed this Aug 14, 2018

onluiz reopened this Aug 14, 2018

alextrastero reviewed Aug 15, 2018

View reviewed changes

onluiz added 3 commits August 15, 2018 13:04

Remove faicon from FixedActionButton example

a72eee0

Replace jquery in Button component

e400c1a

Add flatbutton to docs

8dc6790

onluiz closed this Aug 16, 2018

alextrastero reviewed Aug 17, 2018

View reviewed changes

onluiz added 3 commits August 20, 2018 08:26

Add default value for tooltipOptions in Button component

6c70944

Add tests for FloatingActionButton in Button component

005f87e

Refactor instance destroy of button component

2247f8a

petergarnaes approved these changes Aug 31, 2018

View reviewed changes

alextrastero reviewed Sep 2, 2018

View reviewed changes

alextrastero added the 3.0.0 label Sep 30, 2018

onluiz added 4 commits October 1, 2018 12:46

Adds master package-lock.json

b0756f1

Remove unnecessary condition at Button component

f1455ee

Merge button docs

589f8f6

Remove nonused page

614dd1d

Checkout modal snapshot from beta branch

50d8f75

onluiz added 2 commits November 5, 2018 13:38

reverting wrong snapshot

02a39ee

Merge branch '3.0.0-beta.0' into 3.0.0-beta.0-Button-Component

9b49e92

Add regenerate snapshot

fa5d968

alextrastero added 2 commits December 14, 2018 17:14

revert unwanted changes

696f65e

Revert "revert unwanted changes"

af5cde6

This reverts commit 696f65e.

alextrastero merged commit 3defb94 into react-materialize:3.0.0-beta.0 Dec 14, 2018

3.0.0 Button.js #632

3.0.0 Button.js #632

Conversation

onluiz commented Aug 13, 2018

Description

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petergarnaes left a comment

Choose a reason for hiding this comment

alextrastero commented Aug 14, 2018

onluiz commented Aug 14, 2018

alextrastero left a comment

Choose a reason for hiding this comment

alextrastero Aug 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onluiz commented Aug 15, 2018

petergarnaes commented Aug 16, 2018 • edited

onluiz commented Aug 16, 2018 • edited

onluiz commented Aug 16, 2018 • edited

alextrastero commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petergarnaes commented Aug 31, 2018

onluiz commented Aug 31, 2018

petergarnaes commented Aug 31, 2018

alextrastero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alextrastero commented Sep 21, 2018

onluiz commented Sep 24, 2018

alextrastero commented Sep 25, 2018

alextrastero commented Nov 1, 2018

onluiz commented Nov 1, 2018

onluiz commented Dec 14, 2018

alextrastero commented Dec 14, 2018

onluiz commented Dec 14, 2018

alextrastero commented Dec 14, 2018

onluiz commented Dec 14, 2018

alextrastero Aug 15, 2018 •

edited

petergarnaes commented Aug 16, 2018 •

edited

onluiz commented Aug 16, 2018 •

edited

onluiz commented Aug 16, 2018 •

edited