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

display add step menu with common modules as "quick" buttons #479

Merged
merged 9 commits into from
Nov 22, 2018
Merged

display add step menu with common modules as "quick" buttons #479

merged 9 commits into from
Nov 22, 2018

Conversation

jonxuxu
Copy link
Member

@jonxuxu jonxuxu commented Nov 18, 2018

Fixes #432

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

Hi there,
I'm competing in CodeIn, and I've made a basic selector with commonly used buttons in response to #432 .
image

Some things I still need help on:

  • Deselect a dropdown option when a button is pressed
  • Send the button values to the addStepUi, or the dropdown values if no button is selected

This will be my first pull request on this repository, so I'd appreciate it if you let me know if there's any issues with my pull request. Thanks for your time!

@welcome
Copy link

welcome bot commented Nov 18, 2018

Thanks for opening this pull request! Dangerbot will test out your code and reply in a bit with some pointers and requests.
There may be some errors, but don't worry! We're here to help! 👍🎉😄

@jywarren
Copy link
Member

Hi, what if clicking the icon buttons just add automatically, without having to press add step, so then the deselect of the drop-down wouldnt matter?

@jonxuxu
Copy link
Member Author

jonxuxu commented Nov 18, 2018

That sounds like a good idea.

$("#addStep select").on("change", function(){
$(".radio-group").find('.radio').removeClass('selected');
ui.selectNewStepUi;
});
$("#addStep #add-step-btn").on("click", function(){
//TODO: Get step option from either buttons or dropdown
ui.addStepUi;
});

Could I get a pointer to how ui.addStepUi works? Also where can I find modulesInfo, specifically the name?

I'm thinking of changing the selected value in the dropdown and call the addStepUi function when a corresponding button is pressed.

@jonxuxu
Copy link
Member Author

jonxuxu commented Nov 20, 2018

@jywarren I've added the action exactly as you described. Now clicking the buttons calls the ui.selectNewStepUi() and ui.addStepUi() functions for their respective filters.

@jywarren
Copy link
Member

jywarren commented Nov 20, 2018 via email

@jonxuxu
Copy link
Member Author

jonxuxu commented Nov 20, 2018

@jywarren here it is.
sequence

@jywarren
Copy link
Member

This looks amazing! OK - last few tweaks -- can you:

  1. set border radius to something larger, like 100px, so that the buttons appear truly circular
  2. move the button text outside so it doesn't have to be inside the circle, but below it
  3. add some margin below the row of circular buttons
  4. make sure the button returns to a normal style once the step is added

Thanks so much, this is tremendous!

@jywarren
Copy link
Member

I'm so excited!!! 🎉 👍 👍 🎈

@jywarren
Copy link
Member

Also, it looks like, since we recently merged in some new changes to the master branch, you need to rebase your work on top of the latest main branch. You can do that by following the process outlined here, but you may be able to skip step 2 if your master is still in sync with publiclab's main branch: https://publiclab.org/wiki/contributing-to-public-lab-software#Rewinding+the+master+branch

Thanks!

@jonxuxu
Copy link
Member Author

jonxuxu commented Nov 21, 2018

I believe that I've applied the changes, but grunt serve is not reflecting any of the changes, whether it be css, js or html. The page loads fine and there are no errors, just that it is loading an old version somehow. Is there a way to resolve this?

@ghost ghost assigned jywarren Nov 21, 2018
@ghost ghost added the in-progress label Nov 21, 2018
@jywarren
Copy link
Member

I just resolved the merge conflicts here, let's see!

@jywarren
Copy link
Member

Also, there is a cache, so you may have to open it in the dev console, and then clearing cache that way.

screenshot 2018-11-20 at 10 08 04 pm

I'll check, there is supposed to be a message in the footer with a link to clear the cache.

@jonxuxu
Copy link
Member Author

jonxuxu commented Nov 21, 2018

Thanks for merging the pull request. Grunt is still serving an old version of the site, after I cleared cache, restarted the browser, reinstalled grunt and restarted my computer. It started yesterday and I'm not sure why.
image
As you can see in the console, it's still referencing an error in demo.js line 163 despite it being only 148 lines long now.

Could it be that gruntfile.js was modified and stopped watching the examples folder?

EDIT: I've managed to get grunt to watch the examples folder for a change there. However it's still loading an old version of the page and no change is reflected. I'm completely lost on how to get grunt working again. 😅
image

@jywarren
Copy link
Member

Hmm, actually it shouldn't need to be watching for changes at all, since demo.js is not compiled... Hmm

@jywarren
Copy link
Member

Did you try clearing cache with the reload button menu as in my screenshot? I am pretty sure the cache is in the browser and not the backend.

Another temporary fix would be to use an incognito window, which wouldn't use the cache.

@jywarren
Copy link
Member

Could you push up your changes and rebase (or merge should be fine too) so once we resolve the issues i can merge this in? Thanks!

@jonxuxu
Copy link
Member Author

jonxuxu commented Nov 21, 2018

Thanks, it turns out Firefox doesn't clear the browser cache even when I manually do it in the settings. In private mode I was able to view the changes. I've applied the changes you've requested:
image

@jywarren
Copy link
Member

This is gorgeous. I think you still need to push up the changes to this branch? And there are still some merge conflicts to be resolved by a rebase. But it should be pretty simple -- tell me if you'd like some help!

@jywarren
Copy link
Member

Oh lol there are your commits!

@jywarren
Copy link
Member

But yes, see if you can resolve the conflicts! ⚡⚡👍🏽

@jonxuxu
Copy link
Member Author

jonxuxu commented Nov 22, 2018

Yep, I just fixed the merge conflicts. I kept the stepmodule stuff in the js.

@jywarren
Copy link
Member

Re-added package-lock.json which had been removed, and then i'll merge! 👍 🎉

@jywarren jywarren merged commit f672dc8 into publiclab:main Nov 22, 2018
@welcome
Copy link

welcome bot commented Nov 22, 2018

Congrats on merging your first pull request! 🙌🎉⚡️
Your code will likely be published to https://sequencer.publiclab.org in the next few days.
In the meantime, can you tell us your Twitter handle so we can thank you properly?
Now that you've completed this, you can help someone else take their first step!
See: Public Lab's coding community!

@ghost ghost removed the in-progress label Nov 22, 2018
@jywarren
Copy link
Member

Publishing to gh-pages so we can try it out! Fantastic!!

@jywarren
Copy link
Member

This is a pretty interesting alternative way to select a step, which you might be interested in as a next issue: #247

it's a big one, but if you'd like, I'm happy to make a GCI issue from it!

@jywarren
Copy link
Member

It's live, by the way!!!! Amazing!!!

@jonxuxu
Copy link
Member Author

jonxuxu commented Nov 22, 2018

Great, thanks for your help throughout! @247 looks cool! I've submitted this task for review. Can't wait to work on my next GCI task.

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.

2 participants