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

Fixed Save Button #562

Merged
merged 28 commits into from
Jan 7, 2019
Merged

Fixed Save Button #562

merged 28 commits into from
Jan 7, 2019

Conversation

harshkhandeparkar
Copy link
Member

@harshkhandeparkar harshkhandeparkar commented Dec 27, 2018

Fixes #340

Save button gets enabled whenever there is a change in the value of the input.

fixed-save-btn-sequencer

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

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

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

tech4GT and others added 2 commits December 27, 2018 19:00
Signed-off-by: tech4GT <varun.gupta1798@gmail.com>

dist update

Revert "dist update"

This reverts commit 9ee2a98.
@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Dec 27, 2018

@tech4GT please have a look at this.

@harshkhandeparkar
Copy link
Member Author

Sorry for the update dist extra commit, I somehow screwed up my local dist files and had to squash some commits.

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Dec 27, 2018

I found out an issue. This doesn't work for select type input, the initial value is different and changes after some time.

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Dec 27, 2018

It also didn't use to update the initValue so saving more than once would break the save buttons. But this is fixed now in 'fix update initValue' commit. Select is still an issue though.

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Dec 27, 2018

Update: I fixed the select type inputs by moving the event listeners to the end of onComplete function.

@harshkhandeparkar
Copy link
Member Author

fixed-select-inputs-sequencer

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Dec 27, 2018

@tech4GT I have a suggestion, Can you open a new issue to change all the module inputs types to match their actual html valid types. For e.g. angle for rotate module is 'integer' but it should be number so that increment and decrement buttons appear on pc browsers. Also for modules like resize the percentage is a 'string' not text.

Copy link
Member

@tech4GT tech4GT left a comment

Choose a reason for hiding this comment

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

@harshkhandeparkar You can go ahead and open the issue, that'll be great! Also can you please share a gif using some module which takes text based input like saturation? Thanks! :)

@harshkhandeparkar
Copy link
Member Author

Text-based input:

text-based-input-save-btn

I tried changing the field type to number but it only gives string type input. For number type inputs, 0.2 and 0.20 are actually considered as different even though they are not.

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Dec 28, 2018

I actually fixed this. Committing in a min

@harshkhandeparkar
Copy link
Member Author

Fix:

fixed-integer-inputs

@harshkhandeparkar
Copy link
Member Author

Hey I found something else. The form can be submitted through enter even though the button is disabled.

@harshkhandeparkar
Copy link
Member Author

Please don't merge this PR. I'm going to fix the enter button submit issue. I'm going to be busy for a few days. Will fix this next year 😋

@harshkhandeparkar
Copy link
Member Author

I had to add the e.preventDefault() since the form reloads the page on submitting by default and adds some useless get methods to the URL.

@harshkhandeparkar
Copy link
Member Author

fix-save-to-enter-1

@harshkhandeparkar
Copy link
Member Author

fix-save-to-enter-2

@harshkhandeparkar
Copy link
Member Author

This solution works but I noticed that it slows down the browser significantly. What should I do?. @publiclab/reviewers please help.

HarshKhandeparkar added 2 commits December 31, 2018 20:40
@harshkhandeparkar
Copy link
Member Author

@tech4GT @jywarren please have a look. Also I wanted to ask if ES6 features like arrow functions and template literals can be used in the example page code?

@harshkhandeparkar
Copy link
Member Author

Everything still works:

Range ✔️

range-still-works

This ⬆️ ⬆️ works even better than earlier 🎉 🎉

Multi-Input ✔️

multi-input-still-works

Select ✔️

channel-still-works

Number ✔️

number-still-works

@harshkhandeparkar
Copy link
Member Author

@jywarren @tech4GT please have a look.

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Jan 6, 2019

@tech4GT @jywarren I have an idea. Can we add a new module which takes text as input and adds a qr code to any place in the image like at the corners etc. Maybe to make a photo with a link or something?

@harshkhandeparkar
Copy link
Member Author

@jywarren can we use es6 in the example page code?

@jywarren
Copy link
Member

jywarren commented Jan 6, 2019

What browsers is es6 compliant in? If almost all, then sure, but if there's an easy alternative, maybe not?

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Jan 6, 2019 via email

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Jan 6, 2019

@jywarren @tech4GT please review this.

@harshkhandeparkar
Copy link
Member Author

@jywarren @tech4GT I have resolved conflicts please review. Also please look at my suggestion above 👆👆

@jywarren
Copy link
Member

jywarren commented Jan 7, 2019

Hi, this is great. I'd like one more person to review it if possible -- maybe @Mridul97 ? Also be aware I'd like to merge this before #627, but after that one we may be able to start adding UI tests, which would be great to preserve and maintain these complex UI interactions. Great work!

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Jan 7, 2019 via email

@harshkhandeparkar
Copy link
Member Author

@jywarren how is my suggestion about a new module above 👆 👆

@jywarren jywarren mentioned this pull request Jan 7, 2019
4 tasks
@jywarren
Copy link
Member

jywarren commented Jan 7, 2019

Hi, sure - perhaps you could open a new issue for that? it could be called add-qr

@harshkhandeparkar
Copy link
Member Author

Hi, sure - perhaps you could open a new issue for that? it could be called add-qr

Thanks for the green signal. I will open the issue tomorrow.

Copy link

@Mridul97 Mridul97 left a comment

Choose a reason for hiding this comment

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

This looks good to me. Great work! :)

@harshkhandeparkar
Copy link
Member Author

Thanks @Mridul97

@jywarren jywarren merged commit 4e5f872 into publiclab:main Jan 7, 2019
@jywarren
Copy link
Member

jywarren commented Jan 7, 2019

Awesome, thank you!!!

@harshkhandeparkar
Copy link
Member Author

Thanks @jywarren
I think #377 also should be closed?

@harshkhandeparkar harshkhandeparkar deleted the fix-save-button branch January 8, 2019 01:52
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.

'Save' button for an action is automatically disabled in UI
4 participants