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

Added line that selects the "More modules..." after adding a step. #706

Closed
wants to merge 7 commits into from
Closed

Conversation

gfting
Copy link

@gfting gfting commented Jan 23, 2019

Fixes #697

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 npm test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/reviewers and @publiclab/is-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!

Mridul97 and others added 3 commits October 26, 2018 13:58
* Add manifest.json

* cache static assets for offline use

* update cache

* add meta theme-color and change static files to be cache

* cache the files on network request

* caching on first run

Signed-off-by: tech4GT <varun.gupta1798@gmail.com>

* add a button to clear cache

* add styling to clear cache link
I've arranged the modules in alphabetical order.
@welcome
Copy link

welcome bot commented Jan 23, 2019

Thanks for opening this pull request!
There may be some errors, but don't worry! We're here to help! 👍🎉😄

@gfting
Copy link
Author

gfting commented Jan 23, 2019

@publiclab/is-reviewers thanks!

@harshkhandeparkar
Copy link
Member

@gfting you don't have to add the green tick in the checkboxes. They can be interacted with

@harshkhandeparkar
Copy link
Member

@publiclab/is-reviewers

@harshkhandeparkar
Copy link
Member

Please share a GIF. Thanks!

@harshkhandeparkar
Copy link
Member

Please share a GIF of this change in action.

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.

Looks good to me. Just run grunt build and commit the image-sequencer-ui.js and image-sequencer-ui.min.js files.

@gfting
Copy link
Author

gfting commented Jan 23, 2019

@harshkhandeparkar here's the GIF and the fix! Thanks!
imagesequencerfix

@aashna27
Copy link

aashna27 commented Jan 23, 2019

Please share a GIF of this change in action.

I guess it's okay , I have tested it locally it works. 🙌
@gfting thanks a lot for the great work, and @harshkhandeparkar you are awesome with your support.

@harshkhandeparkar
Copy link
Member

@gfting awesome.

@gfting
Copy link
Author

gfting commented Jan 23, 2019

@harshkhandeparkar your support was seriously incredible during this - thank you very much!

dist/image-sequencer-ui.js Outdated Show resolved Hide resolved
@gfting
Copy link
Author

gfting commented Jan 24, 2019

@rexagod I'm not sure what's going on with this branch and conflicts - what should I do? Thanks!

@harshkhandeparkar
Copy link
Member

harshkhandeparkar commented Jan 24, 2019 via email

@harshkhandeparkar
Copy link
Member

@gfting
Copy link
Author

gfting commented Jan 24, 2019

@harshkhandeparkar when I run the command, I get the error message
merge: upstream - not something we can merge - what should I do?

@rexagod
Copy link
Member

rexagod commented Jan 24, 2019

@gfting Try these above suggestions by @harshkhandeparkar, and feel free to let us know if this works out for you or not. We're here to help! ;)

@harshkhandeparkar
Copy link
Member

@gfting you will have to add the remote
git remote add upstream https://github.com/publiclab/image-sequencer.git

and then merge git merge upstream/main

The /main is necessary.

@gfting
Copy link
Author

gfting commented Jan 24, 2019

@harshkhandeparkar I followed that, but still got the same error message; ended up doing git fetch and then trying to merge

@harshkhandeparkar
Copy link
Member

@gfting did you merge upstream/master or jywarren/main ? Your PR seems to be a a lot of commits behind master. It seems like instead of updating your branch, you did something else. Please try to fix that by merging the proper upstream. You can ask for help here.

@gfting
Copy link
Author

gfting commented Jan 24, 2019

I'm not sure what happened here - I merged upstream master

@harshkhandeparkar
Copy link
Member

@gfting master is not the branch you need to merge, the default branch for image sequencer is main

@harshkhandeparkar
Copy link
Member

Master is 100s of commits behind main

@harshkhandeparkar
Copy link
Member

What you can do is

  • copy your changes and save them in some local file
  • delete your local repo
  • delete the fork
  • fork again
  • clone the fork
  • run npm run setup
  • create a new branch
  • copy your changes
  • close this PR for now
  • open a new one later on

This is why you should always create a new branch for doing such things so that you can fallback to the main brach in case such things happen. Sorry you have to take a lot of trouble to do this.

@gfting
Copy link
Author

gfting commented Jan 24, 2019

It says that I'm already up to date with main

@harshkhandeparkar
Copy link
Member

@gfting I dont know why it says that. Did you setup the remote correctly? In any case though, you will have to close this one I guess.

Or you can revert the last few commits

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.

5 participants