Migrate existing buttons to vuetify #379

gabrielademoraes · 2019-09-24T02:34:44Z

Proposed Changes

It was done the migration of the accessible buttons to vuetify. #376

Type of Changes

What type of change this Pull Request brings to Falko?
Put an x in the boxes that apply

Bugfix
New Feature

Checklist

Put an x in the boxes that apply. If you're confused about any of the following topics, ask us. We're here to help you!

This Pull Request has a significant name.
The build is okay (tests, code climate).
This Pull Request mentions a related issue.

Co-authored-by gabrielademoraes <[email protected]>

Co-authored-by: guilhermesiqueira <[email protected]>

MatheusRich · 2019-10-10T14:50:12Z

Hi @gabrielademoraes! Thanks for you contribution. That's quite a lot of changes here. There are many little things to address here, though. I've added some comments, but most of them are one of these categories:

Hardcoded colors
Indent tags with many attributes

Call me again once you fix this (or if you need help).

gabrielademoraes · 2019-10-10T20:40:10Z

First, thanks for the feedback but I still have some doubts... I'm having trouble to see the comments you added, so, could you give some examples, please ?

MatheusRich · 2019-10-10T14:40:40Z

src/components/Authentication/Register.vue

@@ -44,7 +44,7 @@
          <p class="text-danger" v-if="errors.has('form-register.password_confirmation')">{{ errors.first('form-register.password_confirmation') }}</p>
        </div>
        <div class="text-center">
-          <button type="submit" :disabled="disableRegisterButton()" class="btn btn-primary falko-button" id="">Register</button>
+          <v-btn type="submit" :disabled="disableRegisterButton()" class="primary falko-button white--text" id="" color="#86B1B1">Register</v-btn>


It's not good to hardcode this color. Use variables instead.

MatheusRich · 2019-10-10T14:41:32Z

src/components/GitHub/GitHubProjects.vue

+        type="button"
+        class="primary falko-button white--text"
+        id="addButton"
+        color="#86B1B1"


MatheusRich · 2019-10-10T14:42:56Z

src/components/GitHub/GitHubProjects.vue

+        v-bind:class="buttonClass()"
+        v-bind:disabled="!this.isGitHubAuthenticated"
+        v-on:click="getRepos()"
+        v-bind:data-toggle="buttonDataToggle()"


Replace all this v-bing:something to the short version :something.
Ex:
:disabled="!this.isGitHubAuthenticated"

MatheusRich · 2019-10-10T14:46:30Z

src/components/GitHub/GitHubProjects.vue

-            >
-              Close
-            </button>
+              <v-btn type="button" :disabled="disableImportButton()" class="primary falko-button white--text" color="#86B1B1" v-on:click="importGithubProjects" data-dismiss="modal">


Indent this block

Suggested change

<v-btn type="button" :disabled="disableImportButton()" class="primary falko-button white--text" color="#86B1B1" v-on:click="importGithubProjects" data-dismiss="modal">

<v-btn

type="button"

:disabled="disableImportButton()"

class="primary falko-button white--text"

color="#86B1B1"

v-on:click="importGithubProjects"

data-dismiss="modal"

>```

MatheusRich · 2019-10-10T14:46:50Z

src/components/GitHub/GitHubProjects.vue

+              <v-btn type="button" :disabled="disableImportButton()" class="primary falko-button white--text" color="#86B1B1" v-on:click="importGithubProjects" data-dismiss="modal">
+                Import
+              </v-btn>
+             <v-btn type="button" class="secondary falko-button white--text" color="#868e96" v-on:click="clean" data-dismiss="modal">


Indent this too

MatheusRich · 2019-10-10T14:47:05Z

src/components/Issues/AddIssue.vue

@@ -1,19 +1,19 @@
 <template>
 	<div class="">
 		<div class="text-center">
-			<button type="button" class="btn btn-info btn-md falko-button" id="addButton" data-toggle="modal" data-target="#addIssueModal">
+			<v-btn type="button" class="info falko-button white--text" id="addButton" color="#86B1B1" data-toggle="modal" data-target="#addIssueModal">


Don't hardcode colors

MatheusRich · 2019-10-10T14:47:38Z

src/components/Issues/EditIssue.vue

@@ -43,8 +43,9 @@
            </div>
          </div>
          <div class="modal-footer">
-            <button type="button" :disabled="errors.has('name') || errors.has('body')" class="btn btn-info btn-md falko-button" v-on:click="editIssue(), setAssignees()" data-dismiss="modal">Save</button>
-            <button type="button" class="btn btn-info btn-md falko-button-grey" data-dismiss="modal">Close</button>
+            <v-btn type="button" :disabled="errors.has('name') || errors.has('body')" class="info btn-md falko-button" 


Fix this indentation

MatheusRich · 2019-10-10T23:26:31Z

@gabrielademoraes can you see them now?

gabrielademoraes · 2019-10-11T02:42:44Z

Yes... thanks! We will fix it as soon as possible!

gabrielademoraes · 2019-10-15T19:24:14Z

We guess all the changes you requested were done.

MatheusRich · 2019-10-17T12:16:46Z

src/components/Authentication/Login.vue

@@ -115,4 +115,20 @@ export default {
  font-size: 14px;
  padding-top: 1px;
 }
+
+.button-test {


Should this really be here?

MatheusRich · 2019-10-17T12:28:18Z

src/components/GitHub/GitHubProjects.vue

+  font-weight: bold;
+}
+
+#closeButton {


The code for this id is basically the same of the id above it. If you want the same style in several places, create a css class and added it, instead of Ids.

MatheusRich · 2019-10-17T12:35:57Z

src/components/Authentication/Login.vue

+  color: #ffffff;
+}
+
+#loginButton {


I see that you repeated this css code in several places. You're still defining the button colors hardcoded, which is not good for maintainability.

Think about this scenario:

You have repeated this hardcoded color through the whole code.

Now you want to change the color of all buttons.

You'll have to update EVERY css file to update the colors.

This isn't a maintainable approach.

Let's think about this other scenario:

You've defined a primary color on the Vuetify config file (you can find instructions here)

You've used the class primary to set all the buttons to the primary color defined previously. Ex:

<v-btn color="primary"> ... </v-btn>

Now you want to change the color of all buttons.

The only thing you need to do is update the color in the Vuetify config file. Ex:

export default new Vuetify({ theme: { themes: { light: { primary: colors.red, // A new color }, }, },

guilhermesiqueira · 2019-10-26T16:23:05Z

No more hardcoded colors, @MatheusRich. We were able to encapsulate the entire application with Vuetify and now it was possible to use "tags" to the colors.

guilhermesiqueira and others added 2 commits September 19, 2019 16:51

Changing logout button

324cf51

Co-authored-by gabrielademoraes <[email protected]>

Finishing migration of buttons.

e270c9b

Co-authored-by: guilhermesiqueira <[email protected]>

MatheusRich requested changes Oct 10, 2019

View reviewed changes

MatheusRich mentioned this pull request Oct 11, 2019

Migrate existing inputs to vuetify #380

Merged

5 tasks

gabrielziegler3 assigned gabrielademoraes Oct 14, 2019

gabrielademoraes and others added 2 commits October 15, 2019 11:23

Fixing indentation of the buttons.

28f93b9

Removing hexadecimal and indenting button code

82c8cfd

MatheusRich requested changes Oct 17, 2019

View reviewed changes

guilhermesiqueira added 4 commits October 22, 2019 19:51

Resolving conflicts

26d8ba9

wrapping vue application with vuetify

29dffe7

Adding Vuetify Light theme with custom colors.

8d555f5

Removing hardcoded colors

513db2f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate existing buttons to vuetify #379

Migrate existing buttons to vuetify #379

gabrielademoraes commented Sep 24, 2019

MatheusRich commented Oct 10, 2019 •

edited

Loading

gabrielademoraes commented Oct 10, 2019

MatheusRich Oct 10, 2019

MatheusRich Oct 10, 2019

MatheusRich Oct 10, 2019

MatheusRich Oct 10, 2019

MatheusRich Oct 10, 2019

MatheusRich Oct 10, 2019

MatheusRich Oct 10, 2019

MatheusRich commented Oct 10, 2019

gabrielademoraes commented Oct 11, 2019

gabrielademoraes commented Oct 15, 2019 •

edited

Loading

MatheusRich Oct 17, 2019

MatheusRich Oct 17, 2019

MatheusRich Oct 17, 2019

guilhermesiqueira commented Oct 26, 2019

Migrate existing buttons to vuetify #379

Are you sure you want to change the base?

Migrate existing buttons to vuetify #379

Conversation

gabrielademoraes commented Sep 24, 2019

Proposed Changes

Type of Changes

Checklist

MatheusRich commented Oct 10, 2019 • edited Loading

gabrielademoraes commented Oct 10, 2019

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

MatheusRich commented Oct 10, 2019

gabrielademoraes commented Oct 11, 2019

gabrielademoraes commented Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilhermesiqueira commented Oct 26, 2019

MatheusRich commented Oct 10, 2019 •

edited

Loading

gabrielademoraes commented Oct 15, 2019 •

edited

Loading