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

Fix insert step and allow "closing" it #655

Merged
merged 23 commits into from
Apr 5, 2019
Merged

Fix insert step and allow "closing" it #655

merged 23 commits into from
Apr 5, 2019

Conversation

harshkhandeparkar
Copy link
Member

@harshkhandeparkar harshkhandeparkar commented Jan 11, 2019 •
edited
Loading

Fixes #640
Fixes #918
Fixes #919

Insert step button no longer appends more than once.

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

Thanks!

Divy123
Divy123 previously requested changes Jan 11, 2019
View reviewed changes
Copy link
Member

@Divy123 Divy123 left a comment

Choose a reason for hiding this comment

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

I have suggested some changes. Please look into it.

@harshkhandeparkar
Copy link
Member Author

@publiclab/is-reviewers please review this.

@jywarren jywarren changed the title Fix insert step Fix insert step and allow "closing" it Jan 14, 2019
@jywarren
Copy link
Member

This looks great. Are we all good to merge here once it's been rebased? Thank you!!!

@harshkhandeparkar
Copy link
Member Author

My exams are starting. So I will not be available until 30th January. I will fix the conflicts later.

@harshkhandeparkar harshkhandeparkar dismissed Divy123’s stale review January 19, 2019 07:58

The changes have been made.

@harshkhandeparkar
Copy link
Member Author

I have merged with main. @publiclab/is-reviewers please review.

Divy123
Divy123 approved these changes Jan 19, 2019
View reviewed changes
Copy link
Member

@Divy123 Divy123 left a comment

Choose a reason for hiding this comment

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

I think we are good to go.
Great work @harshkhandeparkar .
This PR is ready to be merged.

jywarren
jywarren approved these changes Jan 22, 2019
View reviewed changes
Copy link
Member

@jywarren jywarren 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! Once rebased and recompiled we can merge it. Thanks!!!

HarshKhandeparkar added 2 commits January 25, 2019 12:15
@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Jan 28, 2019 •
edited
Loading

There is no more lag since the previews are added only after the insert step container has collapsed. @jywarren @publiclab/is-reviewers please review

edit: gif removed

@harshkhandeparkar
Copy link
Member Author

@jywarren can you please merge my PRs before the other ones since I will not be able to touch my PC until 16th of april. I don't know if I will get time past that date as well.

@jywarren
Copy link
Member

jywarren commented Mar 2, 2019

Hi! I'm merging this, but I'd like to suggest we put the "close insert step" button in the Insert Step box itself, because where it is now is a bit confusing -- not clear that you're closing the Insert Step box. I think it's OK for now but will open a new issue and hopefully we can get this modified!

Great work @harshkhandeparkar !!!

...

Hmm, actually now that I am trying to rebase it, I see that some of this was solved in #712, and I am not sure how to proceed here.

I'm really sorry @harshkhandeparkar but I think I need to ask you and @Mridul97 to try to figure this out before we can merge here. I can't quite tell what is the "close box" button code and what is the "fixing insert step" part. 🙁 Sorry!!!

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Mar 3, 2019 •
edited
Loading

@jywarren my PR changes the intermediateHtmlStepUi.js file whereas #712 fixes defaultHtmlStepUi.js where the other file was not required properly.

@publiclab publiclab deleted a comment from Divy123 Apr 3, 2019
@publiclab publiclab deleted a comment from Divy123 Apr 3, 2019
@publiclab publiclab deleted a comment from Divy123 Apr 3, 2019
@publiclab publiclab deleted a comment from Divy123 Apr 3, 2019
@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Apr 3, 2019 •
edited
Loading

I just deleted some old comments and removed old imgs because the page was taking too long to load

@harshkhandeparkar
Copy link
Member Author

@publiclab/is-reviewers please review :)

@harshithpabbati
Copy link

@harshkhandeparkar just one more change after changing above mentioned one. It's about the quick add buttons ! They are not working.

@harshithpabbati
Copy link

@jywarren I have an idea of removing the quick add buttons in the insert-step we can just have the selectize one in the insert step. What are your thoughts about it ?

@harshkhandeparkar
Copy link
Member Author

@Divy123 will have to do that. I don't know how to fix it.

@harshkhandeparkar
Copy link
Member Author

Divy had fixed it for the main selector. It is some problem with selectize.

@harshkhandeparkar
Copy link
Member Author

@jywarren what about this? Will I have to rebase it again because of grid overlay?

@harshithpabbati
Copy link

@harshkhandeparkar no need there is no conflict right???

@harshkhandeparkar
Copy link
Member Author

I was asking if I'll have to rebase. @jywarren please try to review and merge this.

harshithpabbati
harshithpabbati approved these changes Apr 3, 2019
View reviewed changes
Copy link

@harshithpabbati harshithpabbati left a comment

Choose a reason for hiding this comment

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

Great work!!

@harshithpabbati
Copy link

@jywarren this is ready to merge. And @Divy123 will fix the quick add buttons.

@Divy123
Copy link
Member

Divy123 commented Apr 3, 2019

Surely I will.

@harshithpabbati
Copy link

@jywarren do you think we need two close buttons ?

@harshkhandeparkar
Copy link
Member Author

Thanks a lot @Divy123

@harshithpabbati
Copy link

@jywarren this is ready to merge 😄

@harshithpabbati
Copy link

A great work by @harshkhandeparkar 👍

@harshkhandeparkar
Copy link
Member Author

You helped me a lot in it. ❤️
@harshithpabbati @Divy123 @jywarren

@harshkhandeparkar
Copy link
Member Author

This wouldn't have been possibke without the help and feedback everyone provided.

@harshithpabbati
Copy link

That's just because of open source 💓

@jywarren jywarren merged commit 2a0eff4 into publiclab:main Apr 5, 2019
@jywarren
Copy link
Member

jywarren commented Apr 5, 2019

SOOOOO GREAAAATTTTT!!!! 🎉 🎉 🎉 🎉 🎉 🎉

@harshkhandeparkar harshkhandeparkar deleted the fix-insert-step branch June 7, 2019 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harshithpabbati harshithpabbati harshithpabbati approved these changes

@Divy123 Divy123 Awaiting requested review from Divy123

@jywarren jywarren Awaiting requested review from jywarren

Assignees
No one assigned
Labels
bug user-interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@harshkhandeparkar @jywarren @harshithpabbati @Divy123 @tech4GT