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

Foundational PR for new UI #1495

Merged
merged 21 commits into from
Jan 8, 2019

Conversation

dlipeles
Copy link
Contributor

@dlipeles dlipeles commented Jan 3, 2019

Foundational PR for new UI

This PR brings a new structure to our app for future front end implementations, as well as brings a number of new technologies to Publishers

Structure

New UI will live in the app/javascript directory, specifically in two locations

  • app/javascript/packs/* will contain boiler-plate code that will be mounted to a view, these files are minimal and serve as a bridge between react and rails.

  • app/javascript/views/* will contain the actual markup, styles, and functionality of our UI, this directory will be structured using the Fractal component system. It goes something like this

--views
----component1
--------subcomponent1
----------------index.tsx
----------------style.ts
----------------spec.ts
--------subcomponent2
----------------index.tsx
----------------style.ts
----------------spec.ts
--------index.tsx
--------style.ts
--------spec.ts

----component2
...etc
----component3
...etc

Technologies

Bringing a number of new technologies to the table

TypeScript

Superset of javascript, as its name implies this will let us add Type-ing to our code which can improve predictability and testing, as well as prevent some errors. It also can also allow the use of intellisense in your IDE.

Styled Components

Popular component focused styling system known also known as css-in-js. Reduces the amount of markup to apply styles, allows for easy dynamic prop-based styling

Jest

Test kit for React, requires minimal configuration, the spec.ts files will be devoted to testing.

Webpack 4.0

Faster and will have more support in the future, good idea to get a jump on the latest version before starting.

Integration Plan

Access

I've created an e-mail whitelist for access to the new UI, that can help us push to GitHub without having users access it before it's ready. The env variable is new_ui_email_whitelist

Integration

The new UI calls for the following components, this could use some discussion but as I see it, we can organize them this way:

-------------------- Not Necessary for CRM 1.1
-- Dashboard
-- Channels
-------------------- Necessary for CRM 1.1
-- Navbar
-- Referrals
-- Payments

So I believe the plan would be implement Referrals, Payments, and Navbar first, then remove the e-mail whitelist, we can update the Dashboard and Channels page once CRM 1.1 is complete.

Note on the navbar, we will temporarily need to include the Add Channel and Tipping Banner buttons until the new channels page is completed.

@dlipeles dlipeles changed the title Foundational PR for new UI [WIP] Foundational PR for new UI Jan 3, 2019
@dlipeles dlipeles changed the title [WIP] Foundational PR for new UI Foundational PR for new UI Jan 4, 2019
@dlipeles dlipeles changed the title Foundational PR for new UI [WIP] Foundational PR for new UI Jan 4, 2019
@dlipeles dlipeles changed the title [WIP] Foundational PR for new UI Foundational PR for new UI Jan 7, 2019
<Container>
<ReferralsHeader/>
<Grid>
<ReferralsCard/>
Copy link
Contributor

@corymcdonald corymcdonald Jan 7, 2019

Choose a reason for hiding this comment

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

Can we name this component ReferralCard as opposed to Referrals? Each of these items is one Referral so I think that would make sense.

Same goes for the other component. I think it makes sense to have the page named the plural "Referrals" but the components be named a singular "Referral"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rethought this, is it okay if we keep as Referrals? Each card will be representing multiple codes, and I also like the consistency of ReferralsX components in the Referrals section

@corymcdonald
Copy link
Contributor

I'm seeing this error in my console on the Referrals Page

Warning: Received `true` for a non-boolean attribute `title`.

app/javascript/views/referrals/referralsCard/index.tsx Outdated Show resolved Hide resolved
@@ -45,34 +45,44 @@
"preset": "ts-jest/presets/js-with-babel"
},
"dependencies": {
"@rails/webpacker": "3.5",
"@babel/preset-react": "^7.0.0",
"babel-core": "7.0.0-bridge.0",
Copy link
Contributor

@corymcdonald corymcdonald Jan 7, 2019

Choose a reason for hiding this comment

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

warning " > @babel/preset-react@7.0.0" has unmet peer dependency "@babel/core@^7.0.0-0".
warning "@babel/preset-react > @babel/plugin-transform-react-display-name@7.2.0" has unmet peer dependency "@babel/core@^7.0.0-0".
warning "@babel/preset-react > @babel/plugin-transform-react-jsx@7.2.0" has unmet peer dependency "@babel/core@^7.0.0-0".
warning "@babel/preset-react > @babel/plugin-transform-react-jsx-self@7.2.0" has unmet peer dependency "@babel/core@^7.0.0-0".
warning "@babel/preset-react > @babel/plugin-transform-react-jsx-source@7.2.0" has unmet peer dependency "@babel/core@^7.0.0-0".
warning "@babel/preset-react > @babel/plugin-transform-react-jsx > @babel/plugin-syntax-jsx@7.2.0" has unmet peer dependency "@babel/core@^7.0.0-0".
warning " > babel-core@7.0.0-bridge.0" has unmet peer dependency "@babel/core@^7.0.0-0".
warning " > @babel/preset-env@7.2.3" has unmet peer dependency "@babel/core@^7.0.0-0".
warning " > webpack-cli@3.2.0" has unmet peer dependency "webpack@4.x.x".
warning " > jest-styled-components@5.0.1" has incorrect peer dependency "styled-components@^2.0.0 || ^3.0.2".
warning " > postcss-cssnext@3.1.0" has unmet peer dependency "caniuse-lite@^1.0.30000697".
warning " > webpack-dev-server@3.1.14" has unmet peer dependency "webpack@^4.0.0".

This causes some unmet peer dependencies. Can we add to our list of dependencies?
@babel/core@^7.0.0-0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this might explain babel/babel-bridge#5 (comment)

app/javascript/views/referrals/referralsHeader/index.tsx Outdated Show resolved Hide resolved
app/javascript/views/referrals/style.ts Outdated Show resolved Hide resolved
@dlipeles
Copy link
Contributor Author

dlipeles commented Jan 7, 2019

I'm seeing this error in my console on the Referrals Page

Warning: Received `true` for a non-boolean attribute `title`.

@corymcdonald Do you have this line in your tslint.json?

"jsx-boolean-value": false

I was able to suppress the error by adding that

@dlipeles dlipeles merged commit afe29b3 into brave-intl:staging Jan 8, 2019
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.

2 participants