feature/UIDS-442 add Button to DS

[jasonbasuil]

closes #442

• Added Primary, Warning, and Danger variants of the Button component
• Updates documentation
• Swaps out old buttons in various stories with new Button component

For cutover into rails-server, I'm most likely going to start with the Warning or Danger variants and just ensure stability of the component and identify any extra cleanup that might need to occur. I assume the Primary variant will take a few passes to update on RS, so can iterate if additional changes need to be made (including other variants that haven't been included yet).

https://www.figma.com/file/471iS3QrpPtXZpS1xbsJ20/Components?version-id=1070608432&node-id=7330%3A39808

[rroppolo]

So exciting! This looks good I just had a few questions for you!

[rroppolo]

Is there a reason this needs to be top level and not scoped by &. within .Button ?

[jasonbasuil]

It definitely should be! Thanks for catching.

[rroppolo]

What is this needed for? Do react-bootstrap components not import their own css? Is this just to get access to the button-variant mixin?

[jasonbasuil]

I do believe that the mixins do come from this import and is needed. Was also referring to the docs that say it's needed to use Bootstraps source sass files.

[rroppolo]

Looks like in the docs they are putting that import in their App.scss file vs. in each individual scss per component.

What do you think about something like we do in rails-server, where we have a separate bootstrap.scss file and import it into theme.scss. Then theme.scss is already imported at the top of every design system component I believe. Similar to what we do on rails server in ui_imports.scss.

This can let us selectively choose what to import from bootstrap based on our usage. For example, I think the only thing we are using right now would be the three required modules:

@import "../node_modules/bootstrap/scss/functions";
@import "../node_modules/bootstrap/scss/variables";
@import "../node_modules/bootstrap/scss/mixins";

which gives us the button-variant mixin. But likely other bootstrap components will require other mixins, so ideally we would have one central import we add at the top of each component scss vs. having multiple.

[jasonbasuil]

I attempted to create a similar bootstraps.scss file that imports into theme.scss in the DS. Was running into a Webpack issue on the rails-server side. I ended up just importing the node modules directly into Button.scss and had no issues. Not sure if that's okay for now since the button-variant mixins are all that is needed right now for this Button component unless there's a fix for the webpack issue I'm not seeing?

As far as tracking, I'm all for just keeping the tracking on the rails-server side. Quickly explored and we should just be able to modify the TrackedButton component to wrap around the Button component. Something where we remove styling from the TrackedButton but keep all event logging responsibilities there and keep our Button purely presentational:

<TrackedButton 
  onClick={this.handleTestClick}
  type="button"
  event={trackingEvents.WORKSPACE_PARTICIPANTS_FILTERS_CLICKED}
  eventData={
    {
      foo: 'bar'
    }
  }
  >
    <Button variant='primary'>ButtonWrappedInTrackedButton</Button>
</TrackedButton>

Webpack error:

ERROR in ./app/javascript/common/layout/layout.scss
08:39:02 webpack.1   | Module build failed (from ./node_modules/mini-css-extract-plugin/dist/loader.js):
08:39:02 webpack.1   | ModuleBuildError: Module build failed (from ./node_modules/postcss-loader/src/index.js):
08:39:02 webpack.1   | SyntaxError
08:39:02 webpack.1   | 
08:39:02 webpack.1   | (52:10) Unknown word
08:39:02 webpack.1   | 
08:39:02 webpack.1   |   50 |       .#{$property-class + $infix + $property-class-modifier} {
08:39:02 webpack.1   |   51 |         @each $property in $properties {
08:39:02 webpack.1   | > 52 |           #{$property}: $value if($enable-important-utilities, !important, null);
08:39:02 webpack.1   |      |          ^
08:39:02 webpack.1   |   53 |         }
08:39:02 webpack.1   |   54 |       }
08:39:02 webpack.1   | 
08:39:02 webpack.1   |     at /Users/jasonbasuil/UserInterviews/rails-server/node_modules/webpack/lib/NormalModule.js:316:20
08:39:02 webpack.1   |     at /Users/jasonbasuil/UserInterviews/rails-server/node_modules/loader-runner/lib/LoaderRunner.js:367:11
08:39:02 webpack.1   |     at /Users/jasonbasuil/UserInterviews/rails-server/node_modules/loader-runner/lib/LoaderRunner.js:233:18
08:39:02 webpack.1   |     at context.callback (/Users/jasonbasuil/UserInterviews/rails-server/node_modules/loader-runner/lib/LoaderRunner.js:111:13)
08:39:02 webpack.1   |     at /Users/jasonbasuil/UserInterviews/rails-server/node_modules/postcss-loader/src/index.js:207:9
08:39:02 webpack.1   | Child mini-css-extract-plugin node_modules/css-loader/dist/cjs.js??ref--8-1!node_modules/postcss-loader/src/index.js??ref--8-2!node_modules/sass-loader/dist/cjs.js??ref--8-3!app/javascript/common/layout/layout.scss:
08:39:02 webpack.1   |     
08:39:02 webpack.1   |     ERROR in ./app/javascript/common/layout/layout.scss (./node_modules/css-loader/dist/cjs.js??ref--8-1!./node_modules/postcss-loader/src??ref--8-2!./node_modules/sass-loader/dist/cjs.js??ref--8-3!./app/javascript/common/layout/layout.scss)
08:39:02 webpack.1   |     Module build failed (from ./node_modules/postcss-loader/src/index.js):
08:39:02 webpack.1   |     SyntaxError
08:39:02 webpack.1   |     
08:39:02 webpack.1   |     (52:10) Unknown word
08:39:02 webpack.1   |     
08:39:02 webpack.1   |       50 |       .#{$property-class + $infix + $property-class-modifier} {
08:39:02 webpack.1   |       51 |         @each $property in $properties {
08:39:02 webpack.1   |     > 52 |           #{$property}: $value if($enable-important-utilities, !important, null);
08:39:02 webpack.1   |          |          ^
08:39:02 webpack.1   |       53 |         }
08:39:02 webpack.1   |       54 |       }
08:39:02 webpack.1   |     
08:39:02 webpack.1   | Child mini-css-extract-plugin node_modules/css-loader/dist/cjs.js??ref--8-1!node_modules/postcss-loader/src/index.js??ref--8-2!node_modules/sass-loader/dist/cjs.js??ref--8-3!app/javascript/participant/messenger/message_sender.scss:
08:39:02 webpack.1   |     
08:39:02 webpack.1   |     WARNING in ./app/javascript/participant/messenger/message_sender.scss (./node_modules/css-loader/dist/cjs.js??ref--8-1!./node_modules/postcss-loader/src??ref--8-2!./node_modules/sass-loader/dist/cjs.js??ref--8-3!./app/javascript/participant/messenger/message_sender.scss)
08:39:02 webpack.1   |     Module Warning (from ./node_modules/postcss-loader/src/index.js):
08:39:02 webpack.1   |     Warning
08:39:02 webpack.1   |     
08:39:02 webpack.1   |     (93:3) end value has mixed support, consider using flex-end instead
08:39:02 webpack.1   | Child mini-css-extract-plugin node_modules/css-loader/dist/cjs.js??ref--8-1!node_modules/postcss-loader/src/index.js??ref--8-2!node_modules/sass-loader/dist/cjs.js??ref--8-3!app/javascript/participant/project_listings/project_listing.scss:
08:39:02 webpack.1   |     
08:39:02 webpack.1   |     WARNING in ./app/javascript/participant/project_listings/project_listing.scss (./node_modules/css-loader/dist/cjs.js??ref--8-1!./node_modules/postcss-loader/src??ref--8-2!./node_modules/sass-loader/dist/cjs.js??ref--8-3!./app/javascript/participant/project_listings/project_listing.scss)
08:39:02 webpack.1   |     Module Warning (from ./node_modules/postcss-loader/src/index.js):
08:39:02 webpack.1   |     Warning
08:39:02 webpack.1   |     
08:39:02 webpack.1   |     (73:3) end value has mixed support, consider using flex-end instead
08:39:02 webpack.1   | Child mini-css-extract-plugin node_modules/css-loader/dist/cjs.js??ref--8-1!node_modules/postcss-loader/src/index.js??ref--8-2!node_modules/sass-loader/dist/cjs.js??ref--8-3!app/javascript/researcher/participant_search/profile/hub_participant_profile_header.scss:
08:39:02 webpack.1   |     
08:39:02 webpack.1   |     WARNING in ./app/javascript/researcher/participant_search/profile/hub_participant_profile_header.scss (./node_modules/css-loader/dist/cjs.js??ref--8-1!./node_modules/postcss-loader/src??ref--8-2!./node_modules/sass-loader/dist/cjs.js??ref--8-3!./app/javascript/researcher/participant_search/profile/hub_participant_profile_header.scss)
08:39:02 webpack.1   |     Module Warning (from ./node_modules/postcss-loader/src/index.js):
08:39:02 webpack.1   |     Warning
08:39:02 webpack.1   |     
08:39:02 webpack.1   |     (71:5) start value has mixed support, consider using flex-start instead
08:39:02 webpack.1   | ℹ ｢wdm｣: Failed to compile.

[rroppolo]

Is this setting some global theme for bootstrap or only relevant to the button component? Does this need to live somewhere else?

[jasonbasuil]

I think it does set it globally so not scoped to the Button component, but we might not actually need this? I set this originally in the beginning when starting to build the component based on documentation, but since we're using the button-variant mixins to basically set all the custom styles for the button, I'm not entirely sure that the $theme-colors is necessary right now unless we have other components that may make use of the global theme?

It seems to me like the order of the Bootstrap import affects the different button variants. So for example, setting a $secondary variable to pink creates a globally available pink btn-secondary (tho not styled properly) as well as a pink <Button variant='secondary'> (styled properly) without using the $theme-colors variables to set it.

Correctly sets styling

$primary: $ux-emerald-600;
$secondary: pink;

/* import bootstrap to set changes */
@import "~bootstrap/scss/bootstrap";

Does not set styling

/* import bootstrap to set changes */
@import "~bootstrap/scss/bootstrap";

$primary: $ux-emerald-600;
$secondary: pink;

Let me know if that doesn't make sense 😅

[jojolee1996]

So excited for this!! Just a couple notes for each variant:

Primary

• Can we / is it necessary to add outline and disabled variants for the small primary button? Same for danger & warning buttons.
• The button color does not change in the Pressed state for outline variants. Same for danger & warning buttons.

Danger

• Can we set border-radius: 0.25 for the small button?
• Are we able to add in leading & trailing icons to all danger buttons? Same for warning buttons.

Warning

• For the default buttons, the border color does not change on hover

[jojolee1996]

Looks great!!

[jasonbasuil]

Currently looking into an issue with this branch that's causing some Modal issues.

This turned out to be more of a local linking issue rather than an issue with this branch specifically. Will need to look into it further later. Tracking here: #481

Warning: react-modal: App element is not defined. Please use `Modal.setAppElement(el)` or set `appElement={el}`. This is needed so screen readers don't see main content when modal is opened. It is not recommended, but you can opt-out by setting `ariaHideApp={false}`. 
    at ModalPortal (http://localhost:8080/packs/js/vendor-d758c945e6c6b429b304.chunk.js:90176:5)
    at Modal (http://localhost:8080/packs/js/vendor-d758c945e6c6b429b304.chunk.js:89810:5)
    at Modal (http://localhost:8080/packs/js/common-9a1bf521ab7bc378865b.chunk.js:1924:20)
    at div
    at DeleteParticipantsModal (http://localhost:8080/packs/js/researcher/participant_search-b5182deadd4beda5bc5a.chunk.js:17080:5)
    at TitledPage (http://localhost:8080/packs/js/shared-70732d3819cae9f1039f.chunk.js:14381:23)
    at ParticipantSearch (http://localhost:8080/packs/js/researcher/participant_search-b5182deadd4beda5bc5a.chunk.js:19662:5)
    at http://localhost:8080/packs/js/shared-70732d3819cae9f1039f.chunk.js:10179:89
    at C (http://localhost:8080/packs/js/vendor-d758c945e6c6b429b304.chunk.js:281921:37)
    at WithTracking (http://localhost:8080/packs/js/vendor-d758c945e6c6b429b304.chunk.js:291707:28)
    at div
    at Route (http://localhost:8080/packs/js/vendor-d758c945e6c6b429b304.chunk.js:281664:29)
    at Switch (http://localhost:8080/packs/js/vendor-d758c945e6c6b429b304.chunk.js:281866:29)
    at div
    at main
    at SidebarView (http://localhost:8080/packs/js/shared-70732d3819cae9f1039f.chunk.js:7852:5)
    at Router (http://localhost:8080/packs/js/vendor-d758c945e6c6b429b304.chunk.js:281299:30)
    at BrowserRouter (http://localhost:8080/packs/js/vendor-d758c945e6c6b429b304.chunk.js:280919:35)
    at ErrorBoundary (http://localhost:8080/packs/js/vendor-d758c945e6c6b429b304.chunk.js:110489:36)
    at UIBrowserRouter (http://localhost:8080/packs/js/shared-70732d3819cae9f1039f.chunk.js:14438:23)
    at Provider (http://localhost:8080/packs/js/vendor-d758c945e6c6b429b304.chunk.js:279191:20)
    at ParticipantSearchRouter (http://localhost:8080/packs/js/researcher/participant_search-b5182deadd4beda5bc5a.chunk.js:20604:5)
    at _class (http://localhost:8080/packs/js/shared-70732d3819cae9f1039f.chunk.js:10279:7)
    at WithTracking (http://localhost:8080/packs/js/vendor-d758c945e6c6b429b304.chunk.js:291707:28)
    at push../app/javascript/common/hoc/with_auth_user_context.jsx.__webpack_exports__.default (http://localhost:8080/packs/js/shared-70732d3819cae9f1039f.chunk.js:9878:89)

[rroppolo]

YAY buttons! Sorry for the delay - I do think there's some things to work through with our css (scss vs. styled components, defining or accessing scss vars in javascript, how we globally define our bootstrap imports so we don't have to reimport) but I think these things are all worthy of a separate effort to resolve and don't necessarily need to hold up the awesomeness that is buttons :)