Customr components #32

Balsam-Faysal · 2018-08-05T11:19:25Z

#30 add all components for customer with style and react events

tomduggan85 · 2018-08-05T13:30:09Z

client/src/components/LabelImg/labelimg.css

@@ -0,0 +1,38 @@
+@media screen and (max-width: 500px) {


Are there any styles for these components that have the @media selectors that you'd want to keep for all screen sizes? Since all of these styles are currently wrapped with the @media, this component will appear unstyled on desktop (or rather, for all screen widths above 500px, i.e. ipad).

The main target for is is mobile but I agree it doesn't have to be mobile only :)
We will fix this.

tomduggan85 · 2018-08-05T13:32:05Z

client/src/components/NavBar/index.js

+  </div>
+);
+
+export default NavBar;


I hate to have annoying name change suggestions, but if you find all of the different Nav components getting a little bit confusing, it might help to name them more specifically, like CustomerPageNavBar and so on. 😀

tomduggan85 · 2018-08-05T13:47:16Z

client/src/customer-pages/pages/MapTap/index.js

+class MapTap extends Component {
+  render() {
+    return (
+      <div className="map-container">


If you're interested, this is another place where you could apply some of these ideas (#25) to put the common code between MapTap and FlightRoute Tap (basically the NavBar and Tabs) into a single file. It would probably require some adjustments to how the classes are currently passed into the Tabs component though.

NouraldinS

If you're doing the backend, DO NOT FORGET THE TESTS @amusameh

NouraldinS · 2018-08-05T22:25:37Z

client/public/index.html

@@ -11,6 +11,7 @@
    crossorigin="anonymous">

  <link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Volkhov">
+  <link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Voces">


Put both imports on lines 13+14 in one line as in

<link href="https://fonts.googleapis.com/css?family=Voces|Volkhov" rel="stylesheet">

NouraldinS · 2018-08-05T22:29:21Z

client/src/components/Card/index.js

+      <div>
+        <div className="title-style">{cardTitle}</div>
+        {info.map(item => (
+          <LabelImg


Warning: Each child in an array or iterator should have a unique "key" prop

NouraldinS · 2018-08-05T22:32:05Z

client/src/pages/customer-pages/FlightInfo/index.js

+        <div className="card-container">
+          <Card
+            cardTitle="Origin"
+            info={[


put this array in a variable/state/props and pass it to the component, do not stack js in your jsx.

Yeah we should do this, but for now, we will finish the admin panel to and later we will start working on customer views, This is balsam work and she made this pull request the last minute before she leave. it wasn't supposed to be in a pull request.

NouraldinS · 2018-08-05T22:33:55Z

client/src/pages/customer-pages/Home/index.js

+
+class Home extends Component {
+  constructor(props) {
+    super(props);


Why are u calling super and passing props to it? You're not using props anywhere else, so you don't need to pass it to the Component constructor.

NouraldinS · 2018-08-06T13:13:44Z

client/src/components/Card/index.js

+    return (
+      <div>
+        <div className="title-style">{cardTitle}</div>
+        {info.map((item, i) => (


Add newline after { and before }, and rename your i variable to index.

The i is upated to index :)

If you mean a space before and space after. We are using prettier to format our code and it disagrees with this

{ "extends": ["react-app", "plugin:prettier/recommended"] }

tomduggan85 · 2018-08-06T13:48:36Z

client/src/components/input/index.js

 }) => (
  <div className="container-input">
-    <label className="label-style">{labelText}</label>
+    <label className={labelClassName}>{labelText}</label>


Here, you could do the same classnames type of fix that you did for the <Select component, or another idea is you could have 'label-style' be the default class and use labelClassName as an override - <label className={labelClassName || 'label-style'}

Either way, the idea is to save you the trouble of passing "label-style" every time you want to create an Input component.

tomduggan85 · 2018-08-06T13:50:01Z

client/src/pages/customer-pages/FlightInfo/flightinfo.css

@@ -0,0 +1,29 @@
+@media screen and (max-width: 500px) {


Here's another spot where some of the styles probably apply to all browser widths, and some others just to max-width: 500px

tomduggan85 · 2018-08-06T13:50:10Z

client/src/pages/customer-pages/FlightMap/flightmap.css

@@ -0,0 +1,23 @@
+@media screen and (max-width: 500px) {


Here's another spot where some of the styles probably apply to all browser widths, and some others just to max-width: 500px

tomduggan85 · 2018-08-06T13:50:28Z

client/src/pages/customer-pages/Home/home.css

@@ -0,0 +1,21 @@
+@media screen and (max-width: 500px) {


Sorry to spam you with all these :) Here's another spot where some of the styles probably apply to all browser widths, and some others just to max-width: 500px

tomduggan85 · 2018-08-06T13:53:47Z

client/src/pages/customer-pages/Home/index.js

+      </SweetAlert>
+    );
+
+    this.setState({


Here, you can store a flag in this.state which says whether to show the SweetAlert component, and then in render(), read that flag and put the SweetAlert component right there in render(), if the flag is true. That way, this.state just has simple (and serializable) data types in it.

tomduggan85 · 2018-08-06T13:55:03Z

client/src/pages/customer-pages/Home/index.js

+            type="checkbox"
+            onTextInputChange={this.handlecheckboxChange}
+          />
+          <Button className="cus-btn-style" onClick={this.popup}>


Should this use this.handleCheck instead of this.popup? I can't find this.popup.

Do we still need this function because we already have this. handleCheck on the whole form?

<Button className="cus-btn-style">

Is this ok?

Ah yep, you're right, you won't need any function at all there.
Your <Button className="cus-btn-style"> looks good to me!

NouraldinS

I cannot review 40+ files and figure out on my own which files belong to which feature, please commit less files.

Balsam-Faysal added 2 commits August 5, 2018 10:44

#30 add all components for customer with style and react events

bb8f56f

#30 add all components for customer with style and react events

b11ed11

tomduggan85 reviewed Aug 5, 2018

View reviewed changes

fix conflict

2e18d70

tomduggan85 reviewed Aug 5, 2018

View reviewed changes

rename some files to make it more readable

a4d7ef7

amusameh requested a review from NouraldinS August 5, 2018 17:08

NouraldinS requested changes Aug 5, 2018

View reviewed changes

Fix: requested changes

bf7b8db

amusameh requested a review from NouraldinS August 6, 2018 06:50

NouraldinS requested changes Aug 6, 2018

View reviewed changes

tomduggan85 reviewed Aug 6, 2018

View reviewed changes

use classnames module and fix some styling ... etc

d58efc6

amusameh requested a review from NouraldinS August 8, 2018 17:31

NouraldinS requested changes Aug 8, 2018

View reviewed changes

amusameh closed this Aug 9, 2018

Balsam-Faysal deleted the customrComponents branch August 9, 2018 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customr components #32

Customr components #32

Balsam-Faysal commented Aug 5, 2018

tomduggan85 Aug 5, 2018

amusameh Aug 5, 2018

tomduggan85 Aug 5, 2018

amusameh Aug 5, 2018

tomduggan85 Aug 5, 2018

NouraldinS left a comment

NouraldinS Aug 5, 2018

amusameh Aug 6, 2018

NouraldinS Aug 5, 2018

NouraldinS Aug 5, 2018

amusameh Aug 6, 2018

NouraldinS Aug 5, 2018

NouraldinS Aug 6, 2018

amusameh Aug 8, 2018 •

edited

Loading

tomduggan85 Aug 6, 2018

tomduggan85 Aug 6, 2018

tomduggan85 Aug 6, 2018

tomduggan85 Aug 6, 2018

tomduggan85 Aug 6, 2018

tomduggan85 Aug 6, 2018

amusameh Aug 8, 2018

tomduggan85 Aug 8, 2018

NouraldinS left a comment

Customr components #32

Customr components #32

Conversation

Balsam-Faysal commented Aug 5, 2018

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

NouraldinS 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amusameh Aug 8, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NouraldinS left a comment

Choose a reason for hiding this comment

amusameh Aug 8, 2018 •

edited

Loading