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.

Sign up for GitHub

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

Jump to bottom

Added Color Picker for selecting RGBA values in modules #1095

Merged
merged 4 commits into from
Jun 16, 2019
Merged

Added Color Picker for selecting RGBA values in modules #1095

merged 4 commits into from
Jun 16, 2019

Conversation

aashna27
Copy link

@aashna27 aashna27 commented Jun 2, 2019 •
edited
Loading

Fixes #597

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/is-reviewers for help, in a comment below
    Screenshot from 2019-06-03 01-06-30

Thanks!

@jywarren
Copy link
Member

jywarren commented Jun 2, 2019 via email

@aashna27
Copy link
Author

aashna27 commented Jun 2, 2019 •
edited
Loading

yeahhh !!! just waiting for the the tests to finish ... really happyy!!!!!! 🎉 🎉

@aashna27
Copy link
Author

aashna27 commented Jun 2, 2019

have also started with input x,y clicks but stuck on how to do it with event-listener.. 😞

@jywarren
Copy link
Member

jywarren commented Jun 2, 2019 via email

@aashna27
Copy link
Author

aashna27 commented Jun 2, 2019 •
edited
Loading

Yeah this is awesome. What about using an input add-on showing a paint palette or something? https://getbootstrap.com/docs/3.3/components/ https://fontawesome.com/icons?q=palette

Yeahh!!!great I ll add a span next to it, idk why the tests are taking so longg.......

@aashna27
Copy link
Author

aashna27 commented Jun 2, 2019 •
edited
Loading

Have indented the defaultHtmlStepUi.js file to clearly identify the else if conditions and also added a span to identify a color-picker input field. @jywarren I went with this as it is also dynamic and changes color instead of the static fontawesome icon. Please have a look. @publiclab/reviewers @publiclab/image-sequencer-guides @publiclab/soc

Screenshot from 2019-06-03 03-39-19

It is perfectly responsive for all sizes and changes.

1

jainaman224
jainaman224 reviewed Jun 4, 2019
View reviewed changes
examples/lib/defaultHtmlStepUi.js Outdated

if(inputDesc.id == 'color-picker'){ // separate input field for color-picker
Copy link

Choose a reason for hiding this comment

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

It would be great. If you can add spaces before (, {, +, += etc. It improves readability. :)

Copy link
Author

Choose a reason for hiding this comment

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

Yup agreed totally!! adding it

examples/lib/defaultHtmlStepUi.js Outdated

if (inputDesc.type.toLowerCase() == 'range') {
html +=
'"min="' +
Copy link

Choose a reason for hiding this comment

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

You have added extra tab by mistake. Small fix.

Copy link
Author

Choose a reason for hiding this comment

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

You have added extra tab by mistake. Small fix.

yeahh I ll modify that, and also I have added indentation as another conditional level I have added.

examples/index.html Outdated
@@ -27,6 +27,7 @@
<script src="../src/ui/prepareDynamic.js"></script>
<script src="../dist/image-sequencer.js" charset="utf-8"></script>
<script src="../dist/image-sequencer-ui.js" charset="utf-8"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/bootstrap-colorpicker/2.5.3/js/bootstrap-colorpicker.js"></script>
Copy link

Choose a reason for hiding this comment

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

When it is ready. Please change src pointing to package in node modules. As I can see you have already included the package.

Good choice of library. 😍

Copy link
Author

Choose a reason for hiding this comment

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

Actually both these things are needed for incorporating the plugin.

Copy link
Member

Choose a reason for hiding this comment

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

Can it not be included through node modules? Isn't the library available in node? @aashna27?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I have made those changes! Thanks a lot 👍 @jainaman224 @harshkhandeparkar

@jainaman224
Copy link

jainaman224 commented Jun 4, 2019 •
edited
Loading

@jywarren I would suggest to use createElement instead of creating it using string (innerHtml). It is almost rewritten. So, it is easier to change now.

As far as I know,

  1. It helps in preserving event handlers and references to existing DOM.
  2. It saves from typo in closing tags.

@aashna27
Copy link
Author

aashna27 commented Jun 4, 2019

@jainaman224 thanks for reviewing!!

@aashna27
Copy link
Author

aashna27 commented Jun 4, 2019

@jywarren I would suggest to use createElement instead of creating it using string (innerHtml). It is almost rewritten. So, it is easier to change now.

As far as I know,

1. It helps in preserving event handlers and references to existing DOM.

2. It saves from typo in closing tags.

What is this in reference to? I did not understand.

@codecov
Copy link

codecov bot commented Jun 7, 2019 •
edited
Loading

Codecov Report

Merging #1095 into main will decrease coverage by 0.07%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1095      +/-   ##
==========================================
- Coverage   56.15%   56.07%   -0.08%     
==========================================
  Files         112      112              
  Lines        2292     2297       +5     
  Branches      359      360       +1     
==========================================
+ Hits         1287     1288       +1     
- Misses       1005     1009       +4
Impacted Files Coverage Δ
src/modules/Tint/Module.js 20% <0%> (-1.06%) ⬇️
examples/lib/defaultHtmlStepUi.js 13.58% <0%> (-0.26%) ⬇️
src/modules/Crop/Crop.js 97.72% <100%> (+0.05%) ⬆️
src/modules/DrawRectangle/DrawRectangle.js 100% <100%> (ø) ⬆️

@jainaman224
Copy link

Sorry I missed this notification. I was stating it regarding DOM manipulation.

Have a look at https://stackoverflow.com/questions/2946656/advantages-of-createelement-over-innerhtml.

@jywarren
Copy link
Member

jywarren commented Jun 9, 2019 via email

@aashna27
Copy link
Author

aashna27 commented Jun 9, 2019 via email

@aashna27
Copy link
Author

I have removed the alpha value from 2 modules as the decimal values are changing to 0 or 1. So we are getting only 2 colors black or white.

@aashna27
Copy link
Author

aashna27 commented Jun 13, 2019 •
edited
Loading

@harshithpabbati @harshkhandeparkar @jainaman224 @rbhatia46 @IgorWilbert if you could review, I ll resolve conflicts after that!

@harshkhandeparkar
Copy link
Member

I will review in a few hours

@harshkhandeparkar
Copy link
Member

@aashna27 I didn't get time to use my PC today. Will it be okay if I review it tomorrow?

@aashna27
Copy link
Author

aashna27 commented Jun 13, 2019 via email •
edited
Loading

harshkhandeparkar
harshkhandeparkar requested changes Jun 15, 2019
View reviewed changes
Copy link
Member

@harshkhandeparkar harshkhandeparkar left a comment

Choose a reason for hiding this comment

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

I guess you cannot check for color-picker type directly as examples/lib/mapHtmlTypes.js maps any non-recognised type to just text. @aashna27

  default:
    htmlType = 'text';
    break;

You will have to edit ⬆️ this too.

@aashna27
Copy link
Author

I guess you cannot check for color-picker type directly as examples/lib/mapHtmlTypes.js maps any non-recognised type to just text. @aashna27

  default:
    htmlType = 'text';
    break;

You will have to edit arrow_up this too.

No we aren't checking for color-picker type, but assigning an id=color-picker the type can be text or string.

@aashna27
Copy link
Author

I guess you cannot check for color-picker type directly as examples/lib/mapHtmlTypes.js maps any non-recognised type to just text. @aashna27

  default:
    htmlType = 'text';
    break;

You will have to edit arrow_up this too.

@harshkhandeparkar I hope the changes you requested have been answered. Would you like to approve this now? Lemme know if there are any other modifications needed. Thanks 😸

@jywarren
Copy link
Member

Awesome!!! Let's resolve conflicts and merge!!!!

@aashna27
Copy link
Author

Awesome!!! Let's resolve conflicts and merge!!!!

Resolved and thanks! 😄

@jywarren jywarren merged commit b34ec00 into publiclab:main Jun 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jainaman224 jainaman224 jainaman224 left review comments

@jywarren jywarren jywarren approved these changes

@harshkhandeparkar harshkhandeparkar harshkhandeparkar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hex color picker input in default HTML UI
4 participants
@aashna27 @jywarren @jainaman224 @harshkhandeparkar