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

Menus #33

Closed
wants to merge 16 commits into from
Closed

Menus #33

wants to merge 16 commits into from

Conversation

aforemny
Copy link
Collaborator

@aforemny aforemny commented Apr 16, 2016

Hi, I made an attempt at implementing menus! :)

It is still work-in-progress, but I wanted to share it. Any chance of getting this merged when it's done?
Note that I am using native code to query a few javascript properties of the menu element on button press. I think I cannot do this in pure Elm, can I?

Demo: https://aforemny.github.io/elm-mdl/#/menus

@debois
Copy link
Owner

debois commented Apr 16, 2016

Hi Alexander,

Awesome! I definitely want to merge this when it's done!

About native code: We can't have native code in elm-mdl, because then we don't get to publish it as a package. But this is not a showstopper, I thin, because you can read Javascript properties off the DOM in pure Elm: you construct a Json.Decoder that traverses the DOM event you get and collect the information you need.

I'm doing a similar thing in the Ripple animation implementation. Since you need to call getClientBoundingRect, you probably need to use this library (ripple animations also use it). Let me know if you get problems with this, I'll be very happy to assist.

Thanks!

Søren

@debois debois mentioned this pull request Apr 16, 2016
@debois
Copy link
Owner

debois commented Apr 16, 2016

I opened an issue (#34) for this, just to signal to other's that the Menu component is already being worked on.

@aforemny
Copy link
Collaborator Author

aforemny commented Apr 17, 2016

Hi!

Thanks for your advice. It made me question my assumption that DOM traversal in Decoder a is somehow limited to e.target. Turns out I /can/ decode

target.nextSibling.childNodes[1].childNodes.

However, childNodes is a NodeList (which is not an array), so I still need the following native code to traverse the childNodes:

 var decodeNodeList = function(decoder) {
    return function(value) {
      if (value instanceof NodeList) { // only change from Json.Native.decodeArray
        var len = value.length;
        var list = List.Nil;
        for (var i = len; i--; ){
          list = List.Cons( decoder(value[i]), list );
        }
        return list;
      }
      crash('a NodeList', value);
    };
  };

I reckon that decodeNodeList could be included in core… I will try to adapt my code on Monday and we'll see from there!

PS. I forgot to include Material.Menu.Oracle in my original PR. I uploaded it for completeness' sake, but it is subjected to change on Monday.

@debois
Copy link
Owner

debois commented Apr 17, 2016

Hi Alexander,

Very cool!

If you can decode

target.nextSibling.childNodes[1].childNodes

couldn't you then traverse childNodes using a decoder which repeatedly tries to decode

target.nextSibling.childNodes[1].childNodes[i]

for i=0 and up until the decoder fails?

I'm really keen to have the menu component not rely on native because elm-mdl with a native component won't be allowed in the package repository.

About getting your nodeList decoder into Core. I don't think this will happen in the short term, for two reasons: (1) I think Core is de facto not looking at PRs while 0.17 is being worked on, and (2) your decoder opens up the broader question of what would actually be the best way for an Elm program to access the DOM. I could imagine some people saying that using JSON decoders the way we are is a hack, and there should be some more principled way, possibly involving a language extension, or simply that we should "wait for 0.17". (I could be wrong about both, though; maybe ask on #core in the elm-slack?)

At any rate, I don't want to wait for any of those things: I need Elm to be useful to me now. So as an interrim solution, can I propose you construct a nodeList decoder using not native but the above hack, then submit a PR for elm-dom?

Looking forward to see more stuff monday!

Søren

@aforemny
Copy link
Collaborator Author

Hello,

I just wanted to let you know that I uploaded some bogus Oracle.elm file. Oops. That line that you quoted, in particular childNodes[1] does not work, and I still need Native to decode childNodes. Sorry for the confusion.

@debois
Copy link
Owner

debois commented Apr 19, 2016

Ok. I'all try to take a look at decoding childNodes it tonight or tomorrow.
The menu itself (with Native for now) is still work in progress, yes?

tir. 19. apr. 2016 kl. 01.10 skrev Alexander Foremny <
notifications@github.com>:

Hello,

I just wanted to let you know that I uploaded some bogus Oracle.elm file.
Oops. That line that you quoted, in particular childNodes[1] does not
work, and I still need Native to decode childNodes. Sorry for the
confusion.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#33 (comment)

Søren Debois
+45 5358 5818

@aforemny
Copy link
Collaborator Author

Yes, it is still work in progress. I will refactor this PR to use Native as we have discussed. I will manage to in the next couple of days. Yesterday something came up. :)

@debois
Copy link
Owner

debois commented Apr 19, 2016

Ok, cool :)

I found a way to decode childNodes and added it to elm-dom. If you update to 1.1.0, you'll find
this decoder. (I exploited that childNodes apparently have fields 0, 1, ... for its elements.)

Was that the last obstacle to getting rid of Native?

@aforemny
Copy link
Collaborator Author

Wow! That's pretty neat! That should solve the need for Native. I will test. :)

@debois
Copy link
Owner

debois commented Apr 20, 2016 via email

@aforemny
Copy link
Collaborator Author

aforemny commented Apr 21, 2016

This is a working implementation. It differs from mdl-menus in at least the following aspects:

  • The hover effect on the button for some reason stays until you click.
  • The menu does not close when clicked outside.
  • I only implemented mouse input.

I did not really get the component model yet, so I removed types and functions related to it.

Can we merge this? In that case I could make it a single commit and open a new PR.

Demo: https://aforemny.github.io/elm-mdl/#/menus

edit: I think I understand the component parts a bit better. I'll try to update it tonight.

{ alignment : Style
, ripple : Bool
, items : Dict Int Ripple.Model
, open : Maybe Bool --
Copy link
Owner

Choose a reason for hiding this comment

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

I'm unsure about the two fields open : Maybe Bool (why Maybe?) and closing : Bool. Looking at the later code, I can't help wondering if it'd be better to encode the possible states directly:

type AnimationState 
  = Idle
  | Opening
  | Open
  | Closing

Would this make the update function clearer?


decode : Decoder Oracle
decode =
object5
Copy link
Owner

Choose a reason for hiding this comment

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

I think you can simply do object5 Oracle, where Oracle is an auto-generated version of the function you define in the next 7 lines.

@debois
Copy link
Owner

debois commented Apr 21, 2016

Altogether: Excellent! Very impressive, especially the Oracle module. I'm very happy you did this!

As you can see, I made some comments, some stylistic, some semantic. I'm hoping you'll take a look and act on the ones that make sense to you.

You mentioned that:

The hover effect on the button for some reason stays until you click.

I don't see that in the demo? However, on Mac OS in both Chrome and Safari, the menu doesn't open on click, only on keyboard input (enter, space).

The menu does not close when clicked outside.

I see that. I wonder why not?

I only implemented mouse input.

Ok.

Can we merge this? In that case I could make it a single commit and open a new PR.

Sure! I'd love it if you'd take a look at some of my other comments first, but otherwise, sure!

edit: I think I understand the component parts a bit better. I'll try to update it tonight.

Ok, cool. If you can make it work, that's be great; otherwise, I can do it later.

@aforemny
Copy link
Collaborator Author

Thanks a lot for your detailed feedback! I'm agreeing with all of it, and will update accordingly. :)

@debois
Copy link
Owner

debois commented Apr 22, 2016

Excellent :)

@aforemny
Copy link
Collaborator Author

aforemny commented Apr 25, 2016

I changed the Action type quite a bit. I wanted to outline what's necessary for the module internally to function.

This implementation does not use the Opening animation state, and does not set the css class is-animating on opening as the mdl implementation does. I cannot see any difference in behaviour, so we might leave it out as it does not require us to have a tick function that changes from Opening to Opened. What's your opinion on this?

Because all actions carry Geometry I would like to replicate them, so that the component's public actions Open, Close and Select Int do not expose Geometry. Do you agree?

I found a fix for the Chrome bug by setting "pointer-events: none" on the menu's i tag. I did not test with Material.Icon yet. I cannot test with Safari unfortunately.

Regarding the fact that the menu does not close when clicked outside: mdl binds to the document's click handler and checks if the click happened outside the menu. Can we similarly subscribe to documents events in Elm?

PS. Html.disabled just did not work, while Html.attribute "disabled" "disabled" does. I did not look into it very much, so I don't know any details.

Demo: https://aforemny.github.io/elm-mdl/#/menus

@debois
Copy link
Owner

debois commented Apr 25, 2016

As I said above: Very neat!

About Opening/is-animating: at least on Chrome, when I try the Demo, the
menu items appear before the background animation is complete. Properly
that's why you need the is-animating and opening state. (Compare with the
mdl implementation here https://getmdl.io/components/#menus-section.)

About hiding the Geometry part of Actions. I think exposing public actions
without them is a good idea. You'd forward the public actions from the
private ones as effects, yes?

Thank you for the Chrome fix—it works nicely now :)

About outside clicks: I don't know of a good way to register clicks with
document. (Quick googling didn't give me anything, your luck might be
better.) But the Layout component has the exact same functionality for its
Drawer; maybe look at what the CSS behind the layout obfuscator
https://github.com/debois/elm-mdl/blob/master/src/Material/Layout.elm#L438-L447
works
and see if you can achieve something similar?

Again, excellent work!

@aforemny
Copy link
Collaborator Author

aforemny commented Apr 30, 2016

Thank you for your detailed comments!

I updated the demo to use the component model, added back the Tick action to menus so that they have the is-animating class set on opening, and hopefully added most of your suggestions. :)

I merged this myself in feabf9f.

@aforemny aforemny closed this Apr 30, 2016
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.

None yet

2 participants