Skip to content
This repository was archived by the owner on Mar 23, 2019. It is now read-only.

Conversation

@sammysamx20
Copy link
Contributor

Fixes #201.

Not completed yet, I wanted you to check code style if you can!

@sammysamx20 sammysamx20 requested a review from helfi92 as a code owner October 26, 2018 23:16
Copy link
Contributor

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

@sammysamx20
Copy link
Contributor Author

@helfi92 Strange it should've worked... I'll push again and see whats going on.

@sammysamx20
Copy link
Contributor Author

@helfi92 It passes!

Copy link
Contributor

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

Let's try using a "fab" button and placing it on the left side of the speed dial button.

screen shot 2018-10-28 at 2 55 11 pm

The code of a fab button will be similar to:

<Tooltip title="Create Hook">
<Button
color="secondary"
variant="fab"
onClick={this.handleCreateHook}
className={classes.actionButton}
>
<PlusIcon />
</Button>
</Tooltip>

@helfi92
Copy link
Contributor

helfi92 commented Oct 30, 2018

How is it going @sammysamx20? Let me know if I can help :)

@sammysamx20
Copy link
Contributor Author

Hey sorry I got busy this last weekend! I should be able to work on this today

@helfi92
Copy link
Contributor

helfi92 commented Oct 30, 2018

No worries! Just wanted to see if everything was okay :)

@sammysamx20
Copy link
Contributor Author

Actually I do have one question, I was planning to wrap the buttons in a div. But I was wondering how you got the hamburger menu down at the bottom right and where the CSS is for that?

@helfi92
Copy link
Contributor

helfi92 commented Oct 31, 2018

I was planning to wrap the buttons in a div

Yes, you can definitely wrap the button in a div. Something like that:

<Tooltip title="Create Task"> 
  <div>
    <Button 
      variant="fab" 
       // ...
    > 
      <PlusIcon /> 
    </Button> 
  <div>
</Tooltip> 

But I was wondering how you got the hamburger menu down at the bottom right and where the CSS is for that?

To have the button have the "circle" shape, you have to provide the variant="fab" prop. In order to have it positioned in the bottom right, you'll need to extend it with a class. Because we do this in many places in our app, we have the following class that you can use: https://github.com/taskcluster/taskcluster-web/blob/master/src/theme.js#L114-L121. You can refer to it via theme.mixins.fab. For example, it is done in

fab: {
...theme.mixins.fab,
},
.

Hope that helps.

@sammysamx20
Copy link
Contributor Author

@helfi92 Sorry for the long delay here's what I have!

@helfi92
Copy link
Contributor

helfi92 commented Oct 31, 2018

Do you mind rebasing on master, otherwise it gets hard to review this patch. There's a couple of ways to do this but I usually do:

# step 1: from your branch, run
git pull https://github.com/taskcluster/taskcluster-web.git master --rebase

# step 2: if there's a file conflict, fix it, then git add. Notice that you don't do git commit after git add here:
git add <file-with-conflict>
git rebase --continue

# step 3: if there's another conflict, go back to step 2

# step 4:  force push your change
git push origin <your-branch-name> --force

@sammysamx20
Copy link
Contributor Author

Should work now with rebase @helfi92

Copy link
Contributor

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

Looks good. Minor changes :)

createdTaskId,
loading,
} = this.state;

Copy link
Contributor

Choose a reason for hiding this comment

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

Add newline.

tooltipOpen
icon={<PlusIcon />}
<Tooltip
style={{ position: 'fixed', bottom: 16, right: 90 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not mix css with javascript. Add the style after

and you can then do:

<Tooltip className={classes.tooltip} ...>

title="Create Task"
>
<Button
color="secondary"
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove color. The color is already specified in classes.createIcon.

@sammysamx20
Copy link
Contributor Author

Let me know if this works.

},
toolTip: {
position: 'fixed',
bottom: 16,
Copy link
Contributor

Choose a reason for hiding this comment

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

bottom: theme.spacing.double,

toolTip: {
position: 'fixed',
bottom: 16,
right: 90,
Copy link
Contributor

Choose a reason for hiding this comment

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

right: theme.spacing.unit * 11,

position: 'fixed',
bottom: 16,
right: 90,
background: '#4CAF50',
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this.

@sammysamx20
Copy link
Contributor Author

When i remove the background attribute CSS, it turns the button white

@sammysamx20
Copy link
Contributor Author

Actually I think I figured it out

Copy link
Contributor

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

Almost there! Added some requested changes. Also, when I click the light theme button (light bulb icon in the app bar), the icon becomes black on green. It should stay the same as the dark theme.

screen shot 2018-11-01 at 8 43 20 am

icon={<PlusIcon />}
<Tooltip
className={classes.createIcon}
placement="left"
Copy link
Contributor

Choose a reason for hiding this comment

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

placement="top".

title="Create Task"
>
<Button
variant="fab"
Copy link
Contributor

@helfi92 helfi92 Nov 1, 2018

Choose a reason for hiding this comment

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

Could you also add requiresAuth, please. We don't want users to create tasks when signed out.

<Button
  requiresAuth
  variant="fab"
  // ...

@sammysamx20
Copy link
Contributor Author

Do you know what is causing the light mode to turn it to black?

@sammysamx20
Copy link
Contributor Author

Actually I think I got it

@helfi92
Copy link
Contributor

helfi92 commented Nov 1, 2018

It's probably

'.mdi-icon': {
fill: theme.palette.text.primary,
},
. In Main.jsx, you can create another style fabIcon with the following style:

+    fabIcon: {
+      '& .mdi-icon': {
+        fill: 'white',
+      },
+    },

Then from the creat task view, you can do:

  createIcon: {
    ...theme.mixins.successIcon,
    ...theme.mixins.fabIcon,
    position: 'fixed',
    bottom: theme.spacing.double,
    right: theme.spacing.unit * 11,
},

I think that should fix it.

@sammysamx20
Copy link
Contributor Author

I pushed some changes let me know if it works!

Copy link
Contributor

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

Looks good to me. A rebase is needed however.

@sammysamx20
Copy link
Contributor Author

is this okay?

Copy link
Contributor

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

LGTM 😍

@sammysamx20
Copy link
Contributor Author

phew I'm glad it works thanks for the help!!

@helfi92
Copy link
Contributor

helfi92 commented Nov 1, 2018

No problem. Awesome work!

icon={<PlusIcon />}
<Tooltip title="Create Task">
<Button
requireAuth
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops. This needs to be requiresAuth 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aw man I'll do that now :(

@helfi92 helfi92 merged commit 9221013 into taskcluster:master Nov 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants