Skip to content

fix uehaj2022/latteart#10#17

Open
imshresth0810 wants to merge 8 commits intomainfrom
10-glowing-recording-button
Open

fix uehaj2022/latteart#10#17
imshresth0810 wants to merge 8 commits intomainfrom
10-glowing-recording-button

Conversation

@imshresth0810
Copy link
Copy Markdown
Collaborator

@imshresth0810 imshresth0810 commented Nov 21, 2022

No description provided.

@imshresth0810 imshresth0810 linked an issue Nov 21, 2022 that may be closed by this pull request
Glowing Recording Button #10
Open
@uehaj2022
Copy link
Copy Markdown
Owner

uehaj2022 commented Nov 24, 2022 •
edited
Loading

Tanks for PR and implementation.
We think your work is good, but be wold like to comment at some point.

  • please change name of pull request to "fix uehaj2022 / latteart#10" ("fix "+organization name+"/latteart#" + issue number)
  • if possible do not change settings of eslint.(current rules are disccuesd).
  • Can you separate PR for the changes for the issue#10, and other changes? To change settings and # 10 issue are different problem, so we hope handle those on dirrerent branch. Especially when we about send the PR to upstream.
  • Could you explan why you change the icon image?
  • Could you explan why you change the color of button red to green? We want to your intent about color cahnge. If those reason are matter of design sense, it is nice if those colors are customizable.
  • 'Comment out' should be removed in the PR.

uehaj2022
uehaj2022 reviewed Nov 24, 2022
View reviewed changes
.eslintrc.js Outdated
@@ -1,3 +1,36 @@
// module.exports = {
Copy link
Copy Markdown
Owner

@uehaj2022 uehaj2022 Nov 24, 2022

Choose a reason for hiding this comment

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

Please remove old code in comment.

src/gui/vue/pages/captureControl/expCapture/organisms/captureToolFooter/AutofillButton.vue Outdated
:title="$store.getters.message('app.autofill')"
>
<v-icon>edit</v-icon>
<!-- <v-icon>edit</v-icon> -->
Copy link
Copy Markdown
Owner

@uehaj2022 uehaj2022 Nov 24, 2022

Choose a reason for hiding this comment

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

Please remove old code on comment.

@imshresth0810 imshresth0810 changed the title push change for issue_10 by gaurav_shresth fix uehaj2022?latteart#10 Nov 24, 2022
@imshresth0810 imshresth0810 changed the title fix uehaj2022?latteart#10 fix uehaj2022/latteart#10 Nov 24, 2022
@imshresth0810
Copy link
Copy Markdown
Collaborator Author

imshresth0810 commented Nov 24, 2022

Good Afternoon Sir,
I've made some changes to your suggestions in the Pull Request.

  1. Name of the PR changes to "fix uehaj2022 / latteart#10" ("fix "+organization name+"/latteart#" + issue number)
  2. Original eslint file updated.
  3. Only changes related to issue#10 are reflected in this PR.
  4. I've been working on dark theme mode, where users can switch between light and dark. So, the color scheme of a green button would be more relevant with the black color.
  5. Comments are removed

uehaj2022
uehaj2022 reviewed Nov 24, 2022
View reviewed changes
tsconfig.json Outdated
@@ -1,7 +1,37 @@
// {
Copy link
Copy Markdown
Owner

@uehaj2022 uehaj2022 Nov 24, 2022

Choose a reason for hiding this comment

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

please remove comment out code.

Copy link
Copy Markdown
Collaborator Author

@imshresth0810 imshresth0810 Nov 24, 2022

Choose a reason for hiding this comment

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

Done

@imshresth0810 imshresth0810 force-pushed the 10-glowing-recording-button branch from 1dca7bf to 1bf2fdc Compare November 24, 2022 12:13
uehaj2022
uehaj2022 reviewed Nov 25, 2022
View reviewed changes
package.json Outdated
"d3": "^5.9.2",
"dayjs": "^1.11.3",
"encoding-japanese": "^1.0.30",
"eslint-config-prettier": "^8.5.0",
Copy link
Copy Markdown
Owner

@uehaj2022 uehaj2022 Nov 25, 2022

Choose a reason for hiding this comment

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

can you remove this module (eslint-config-prettier)?
File formatting policy is discussed and already shared with original project members, so if you want to change it, discussion is needed.

Copy link
Copy Markdown
Collaborator Author

@imshresth0810 imshresth0810 Nov 25, 2022

Choose a reason for hiding this comment

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

Done

package.json Outdated
"vue-chartjs": "^3.4.2",
"vue-class-component": "^7.2.3",
"vue-i18n": "^8.11.2",
"vue-material-design-icons": "^5.1.2",
Copy link
Copy Markdown
Owner

@uehaj2022 uehaj2022 Nov 25, 2022

Choose a reason for hiding this comment

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

could you remove this module?
If you want to use icon, basically choose it from already using module:
https://v15.vuetifyjs.com/en/components/icons

if you have specific reason to use another icon set, please tell us.

Copy link
Copy Markdown
Collaborator Author

@imshresth0810 imshresth0810 Nov 25, 2022

Choose a reason for hiding this comment

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

Done

src/gui/vue/pages/captureControl/expCapture/organisms/captureToolHeader/RecordButton.vue
flat
large
color="red"
color="green"
Copy link
Copy Markdown
Owner

@uehaj2022 uehaj2022 Nov 25, 2022

Choose a reason for hiding this comment

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

We would like to know why you chose green. (For example, did you think green would be easier to understand, etc.)
If it is simply about your color preferences, then it would be better to make the colors customizable in the settings, because preferences can vary from user to user.

Copy link
Copy Markdown
Collaborator Author

@imshresth0810 imshresth0810 Nov 25, 2022

Choose a reason for hiding this comment

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

I choose green because the users more easily recognize it. I'll try to make it customizable in the settings instead of hard-coded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@uehaj2022 uehaj2022 uehaj2022 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Glowing Recording Button

2 participants

@imshresth0810 @uehaj2022