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 area and location #22

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Added area and location #22

wants to merge 2 commits into from

Conversation

dirz88
Copy link

@dirz88 dirz88 commented Dec 29, 2015

The idea with the Area I took over from JimJamUrCode.

  • Added menu bar for improved user experience (pushy.js)
  • Added area: Each macro does have now a location

This is my first project with Node.js or generally in programming. I
hope it meets the requirements and tell me what I need to adjust, it is
adopted.

13 passing (4s)
2 failing

  1. lirc_web routes should have GET route for JSON list of commands for macro:
    Error: expected 200 "OK", got 404 "Not Found"
    at Test._assertStatus (node_modules/supertest/lib/test.js:232:12)
    at Test._assertFunction (node_modules/supertest/lib/test.js:247:11)
    at Test.assert (node_modules/supertest/lib/test.js:148:18)
    at Server.assert (node_modules/supertest/lib/test.js:127:12)
    at emitCloseNT (net.js:1525:8)

  2. lirc_web index action should return an HTML document in which all button elements of class command-link have an href of the form /remotes/:remote/:command:
    Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.

Unfortunately, I still have an error while executing the test..

lirc_overwview
lirc_menu

- Added menu bar for improved user experience (pushy.js)
- Added area: Each macro does have now a location

This is my first project with Node.js or generally in programming. I
hope it meets the requirements and tell me what I need to adjust, it is
adopted.
@alexbain
Copy link
Owner

Thanks for the work on this, @dirz88.

Couple thoughts:

  1. What if you wanted the same macro to work in two different areas? Or, what if certain remotes were only applicable in certain areas? I think it might be more flexible if we introduced areas as a top level configuration option which contained an array of macros and remotes that you wanted to be shown for that area.

    Example config.json:

    "areas": {
    "Office": {
      "macros": [
        "Listen to Music"
      ],
      "remotes": [
        "Yamaha",
      ]
    },
    "Living Room": {
        "macros": [
          "Listen to Music",
          "Play Xbox 360"
        ],
        "remotes": [
          "TV",
          "Xbox 360",
          "Yamaha"
        ]
    }
    }
    
  2. I believe the two tests are failing because you changed the macros route, here: https://github.com/alexbain/lirc_web/pull/22/files#diff-0364f57fbff2fabbe941ed20c328ef1aR124.

  3. I like the introduction of a side menu. I'd been thinking about that myself, and pushy.js looks good.

  4. If areas was a top level configuration option the UI would need to be changed to only show areas if that configuration option was set. Otherwise, it would need to only show macros and remotes

If you're willing to make these changes I'd love to get this added to the project.

@dirz88
Copy link
Author

dirz88 commented Jan 5, 2016

Hello @alexbain

I'll try to integrated your thoughts. At the moment I'm quite busy, but I try to do that as soon as possible :).

@dirz88
Copy link
Author

dirz88 commented Jan 17, 2016

I'm sorry, i had some troubles with git to sync/merge. I accidentally requested further commits and then delete the repository :(. Apologies the troubles..

Actually I've changed all to your wishes expect point 4. I don't know how to implement this - still searching for a solution. But I wonder if this is necessary - since at least 1 room is always required.

main
sidemenu

I've changed the menubar to show the rooms and their remotes.
I would ask for a new commit, if you agree - or should I try to integrate your point 4 first?

Greetings from switzerland,
dirz88

@alexbain
Copy link
Owner

@dirz88 Thanks for updating this. I'll take a look soon and let you any additional thoughts I've got.

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