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.

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

Text Alignment made easy and convenient. #1804

Merged
merged 4 commits into from
Feb 15, 2021

Conversation

vivek-30
Copy link

@vivek-30 vivek-30 commented Feb 5, 2021

Fixes #1767 .
After this fix we can now align text easily at any position on image by just clicking on that point on the image.

@vivek-30 vivek-30 requested review from a team as code owners February 5, 2021 08:47
@gitpod-io
Copy link

gitpod-io bot commented Feb 5, 2021

@vivek-30
Copy link
Author

vivek-30 commented Feb 5, 2021

@harshkhandeparkar ,@jywarren could you please review this.

@vivek-30
Copy link
Author

vivek-30 commented Feb 5, 2021

This is the working Gif for the mentioned issue -
text_alignment

@vivek-30
Copy link
Author

vivek-30 commented Feb 5, 2021

if Gif is hazzy then you can look at this clip -

Screen.Recording.2021-02-05.at.2.04.28.PM.mov

@vivek-30
Copy link
Author

vivek-30 commented Feb 6, 2021

@harshkhandeparkar Could you please check this out ☺️

},
"custom": {
"type": "select",
"desc": "Fill Coordinates On Click.",
Copy link
Member

Choose a reason for hiding this comment

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

This is very cool! Could we change this to a button with a crosshairs icon, maybe? What do you think...

https://fontawesome.com/v4.7.0/icon/crosshairs

<i class="fa fa-crosshairs"></i>

Copy link
Author

Choose a reason for hiding this comment

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

Sure ,that will be a better choice

Copy link
Author

Choose a reason for hiding this comment

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

@jywarren please have a look at these new changes

@vivek-30
Copy link
Author

vivek-30 commented Feb 7, 2021

This is the working clip with new changes -

Screen.Recording.2021-02-07.at.5.22.52.PM.mov

is this ok now ☺️

@vivek-30 vivek-30 force-pushed the text-alignment branch 2 times, most recently from 059499c to cc6160c Compare February 7, 2021 15:14
@daemon1024
Copy link
Member

This is awesome. There's this wierd bug I am facing tho. I need to make some changes in parameters other than the coordinates to make it apply everytime.

@jywarren
Copy link
Member

Hmm, even if you change the coordinates manually instead of by clicking?

@vivek-30 this looks great. Could you put the button leftmost on the line and use non-bold text that says click to select coordinates on the right side, and place it just above the manual coordinates? Thank you!!!

@vivek-30
Copy link
Author

@jywarren , @daemon1024 sir plz have a look now 😊

examples/lib/defaultHtmlStepUi.js Outdated Show resolved Hide resolved
examples/lib/defaultHtmlStepUi.js Outdated Show resolved Hide resolved
src/modules/TextOverlay/info.json Outdated Show resolved Hide resolved
src/modules/TextOverlay/info.json Outdated Show resolved Hide resolved
@harshkhandeparkar
Copy link
Member

switch(inputInfo.type.toLowerCase()){
case 'integer':
htmlType = inputInfo.min != undefined ? 'range' : 'number';
if (htmlType === 'range') inputInfo.step = inputInfo.step || 1; // default range step size for integer
break;
case 'string':
htmlType = 'text';
break;
case 'select':
htmlType = 'select';
break;
case 'percentage':
htmlType = 'number';
break;
case 'float':
htmlType = inputInfo.min != undefined ? 'range' : 'text';
if (htmlType === 'range') inputInfo.step = inputInfo.step || 0.1; // default range step size for float
break;
default:
htmlType = 'text';
break;
}

button is not an actual type here. So we can just add coordinate-input as a type ig. What do you think?

@vivek-30
Copy link
Author

ok you mean we can add type to be coordinate-input and then create another case in switch statement to handle it accordingly?

@harshkhandeparkar
Copy link
Member

ok you mean we can add type to be coordinate-input and then create another case in switch statement to handle it accordingly?

Yes. What do you think we should do?

@vivek-30
Copy link
Author

sure that will be a better way to handle it 😄 .Thanks for suggesting this.

@daemon1024
Copy link
Member

daemon1024 commented Feb 10, 2021

Hmm, even if you change the coordinates manually instead of by clicking?

Nope

Screencast-from-02-11-2021-12_06_28-AM.mp4

As you can see it was not letting me apply changes unless I manually typed in coordinates again or change some other parameter.

@daemon1024
Copy link
Member

daemon1024 commented Feb 10, 2021

@vivek-30 It would be great if you commit each of your changes separately instead of amending the original commit would be easy to understand what changes exactly you made after a review/suggestion. Thank You.

@@ -8,6 +8,10 @@
"desc": "Enter the text to overlay.",
"default": "Lorem ipsum"
},
"Custom coordinates": {
"type": "coordinate-input",
"id": "coordinates-input"
Copy link
Member

Choose a reason for hiding this comment

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

id is no longer required :)

Copy link
Member

Choose a reason for hiding this comment

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

Also, please add a description desc if possible.

Copy link
Author

Choose a reason for hiding this comment

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

i have added that but @jywarren wants non-bold text instead.
should i add desc back now?

@@ -118,7 +118,14 @@ function DefaultHtmlStepUi(_sequencer, options) {
paramVal + '">' + '<span class="input-group-addon"><i></i></span>' +
'</div>';
}
else { // Non color-picker input types
else if(inputDesc.id === 'coordinates-input'){
Copy link
Member

Choose a reason for hiding this comment

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

You can use inputDesc.type instead of id here.

Copy link
Author

Choose a reason for hiding this comment

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

ok i will remove that.

@vivek-30
Copy link
Author

@daemon1024 i thought it will help the maintainers to merge as they themself don't have to do squashing. apologies,😅 now i will take care of this by not amending my commits.

@harshkhandeparkar
Copy link
Member

@daemon1024 i thought it will help the maintainers to merge as they themself don't have to do squashing. apologies, now i will take care of this by not amending my commits.

Oh, we don't do that. Github does it for us :)
image

@vivek-30
Copy link
Author

@daemon1024 i thought it will help the maintainers to merge as they themself don't have to do squashing. apologies, now i will take care of this by not amending my commits.

Oh, we don't do that. Github does it for us :)
image

ok i will take care of it ,actually i did not knew about it.

@vivek-30 vivek-30 requested a review from a team as a code owner February 10, 2021 19:32
@vivek-30
Copy link
Author

sorry for re-pushing this i have accidentally staged and committed some untracked files.

@jywarren
Copy link
Member

No problem @vivek-30 -- all good, and we really appreciate your work on this! Is it ready for a final review? Thanks!

@vivek-30
Copy link
Author

@harshkhandeparkar is there anything else i have to change?

@vivek-30
Copy link
Author

@harshkhandeparkar , @jywarren is this PR ready to be merged😅.

@jywarren jywarren merged commit ed94a0f into publiclab:main Feb 15, 2021
@jywarren
Copy link
Member

Great work, thank you!!!

@vivek-30
Copy link
Author

@jywarren welcome sir 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make text alignment easier with a few more buttons
4 participants