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

Macro UI #87

Merged
merged 206 commits into from Sep 13, 2016

Conversation

Projects
None yet
4 participants
@laxu
Copy link
Contributor

commented Aug 28, 2016

Added partially working macro UI for #73 . Mainly looking for input on what should be done differently as I haven't worked with Angular 2 before or if I'm doing something the wrong way or not using some existing component where I should.

Macro editing popover UI apes key remap UI so it should feel familiar and consistent to use.

What works:

  • Loading existing macro from UHK config file
  • Listing macro actions
  • Editing macro name
  • Editing macro action
  • Deleting macro action
  • Saving macro action in UI and displaying the updates in macro actions list
  • Editing "Delay" type macro action
  • Editing "Text" type macro action
  • Adding new macro to sidebar list

Added features:

  • Popover UI for editing macro action
  • Contenteditable component for editing macro name
  • More icon options to icon component
  • clone method to MacroAction. It creates a copy of the MacroAction, used to edit the macro action without changing the original.
  • Placeholders for all macro action types to MacroItemComponent

Changed:

  • MacroItemComponent icons for various actions to better represent the action. Less black squares. :)

What doesn't work:

  • New added macro cannot be edited/loaded (it's not added to the correct location to be loaded in MacroComponent).
  • Editing key press/hold/release macro actions (needs component taking macro functionality into account).
  • Editing any mouse macro actions (needs component taking macro functionality into account)
  • Changing type of macro being edited. For example if it's delay type and you click text tab it should be changed to text type. Should probably just use the toJsObject method of MacroAction to handle data in the popover as generic data before turning it back to a real MacroAction upon save.
  • Saving macros (to the keyboard I suppose) where they can be loaded again when coming back to Agent.
  • Bug in ng2-slider-component ignores the stepValue so delay slider doesn't move at 0.1s steps like it should.

@laxu laxu referenced this pull request Aug 28, 2016

Closed

Macro component #73

@@ -0,0 +1,118 @@
import {Component, OnInit, OnChanges, Input, Output, EventEmitter, ViewChild } from '@angular/core';
import {NgIf, NgSwitch, NgSwitchCase } from '@angular/common';

This comment has been minimized.

Copy link
@fjozsef

fjozsef Aug 28, 2016

Member

Importing these not needed, these directives are enabled in all components because they are part of the BrowserModule.

require('./macro-popover.component.scss')
],
host: { 'class': 'popover macro-action' },
directives: [

This comment has been minimized.

Copy link
@fjozsef

fjozsef Aug 28, 2016

Member

Don't add directives here. It is deprecated since rc.5, add them in app.module.ts

@fjozsef

This comment has been minimized.

Copy link
Member

commented Aug 28, 2016

@mondalaci Maybe you want to check the UI out.

From the angular2 side I think the code is okay more or less.
Just one thing before I forget: I would move the MacroItemComponent to under the components/macro folder.
Keep working on! :)

(If I may suggest to rebase before you continue your work.)

@mondalaci

This comment has been minimized.

Copy link
Member

commented Aug 28, 2016

Thank you for your contribution, @laxu! Yours is a definite step forward, and it's great to be able to interact with the macro editor.

Although the mockup of the macro UI is not fully ready yet, back in the days we agreed with Arpi that we shouldn't use a popover for this purpose. Please let me make some mockups.

Other than that, some comments:

  • We shouldn't use Font Awesome anymore but IcoMoon Ultimate.
  • The macro editor should be implemented as a standalone component because it will be reused elsewhere in viewer mode. (I'm not sure how it's implemented right now but this is an important point.)
  • The pencil icon shouldn't be featured next to the macro name above.
  • The macro name above should have a dotted underline just as in the case of keymap names and abbreviations. The text should be editable the same manner, and the same styles should apply. The interaction is the same when editing a title of a Google Document.

Gonna follow up regarding the macro UI mockup.

@fjozsef

This comment has been minimized.

Copy link
Member

commented Sep 11, 2016

The list of macro-items should be scrollable when there are too many of them.
Try with adding display: block; height: 100%; to macro and height: 100%; to its child and overflow: auto;
height: 100%; to the second div.
(You may need to add other styles, too. These are just my first guesses.)

@fjozsef

This comment has been minimized.

Copy link
Member

commented Sep 11, 2016

@laxu "Add new action item" should be the part of the scrollable area and add some space after it.

@laxu

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

I disagree about the add new item being part of the scrollable area. This way it is always visible so you don't have to scroll all the way down the list to access it. The list also scrolls down to the editor if you add a new action so I'd consider it to have better usability this way.

I added a few breakpoints so that the size of the scrollable list changes accordingly and leaves some space at the bottom in various window sizes.

@fjozsef

This comment has been minimized.

Copy link
Member

commented Sep 12, 2016

You're right. It is better if it's not the part of it. I just wanted it to be visible when there is a long list.

@laxu

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

Yeah that was my bad for not checking how it handles on much smaller window size than what I normally use.

Merge pull request #2 from fjozsef/refactor/macro
Added: missing return type
@fjozsef

This comment has been minimized.

Copy link
Member

commented Sep 12, 2016

Unused empty space marked with yellow. It should be filled.

empty space

@laxu

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

I left the space like that because of the fork me on github banner. Otherwise it will overlap the content, possibly hiding the edit button for example.

@fjozsef

This comment has been minimized.

Copy link
Member

commented Sep 12, 2016

It should overlap, the banner will not be there forever. Otherwise if we want it to not overlap, then it should be done on higher level in DOM.

@laxu

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

So you want it to go over the banner? Or allow the macro list to go under the banner?

@fjozsef

This comment has been minimized.

Copy link
Member

commented Sep 12, 2016

Second option, under the banner

Mikko Lakomaa added some commits Sep 12, 2016

margin-bottom: 0;
border-radius: 4px 4px 0 0;
border: 1px solid #ddd;
border-bottom: 0;

@media screen and (min-height: 1000px) {
@media screen and (min-height: 730px) {

This comment has been minimized.

Copy link
@fjozsef

fjozsef Sep 12, 2016

Member

These are not working for me, otherwise I think it should be done with flexbox, instead of media query.

@laxu

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

Tried flexbox but couldn't get it to work as expected in this case. Because the main content area is not scrollable content will go outside the window so I setup the breakpoints using vh values and that seemed to be the only way to keep the content as desired. If you can figure out a better way please make a pull request.

fjozsef and others added some commits Sep 12, 2016

Merge pull request #3 from fjozsef/refactor/macro
Fix: Macro list scrolling

@fjozsef fjozsef merged commit fbb4a1c into UltimateHackingKeyboard:master Sep 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@NejcZdovc NejcZdovc referenced this pull request Oct 21, 2016

Merged

Macro items toggle #124

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.