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

Add support for id and inner attributes #206

Merged
merged 4 commits into from
Aug 27, 2016
Merged

Conversation

vipentti
Copy link
Collaborator

Resolves #204

Options.id allows users to specify the id that can then be
used with the new DOM API.
Options.inner allows users to specify attributes on inner
elements of components that support it.

I went through the list by @debois in #204. Only components that I really found needing support for Options.inner were Textfield as well as Toggles, both of them have input elements that the users can now add attributes to with Options.inner.

Most of the other components have users specify sections of content already and for those they can just use Options.id (such as Tabs and Grids). For other components there was really no clear inner element that would benefit from enabling this support.

As always looking for feedback and suggestions, especially if there is a component that could really use Options.inner support

`Options.id` allows users to specify the `id` that can then be
used with the new DOM API.
`Options.inner` allows users to specify attributes on `inner`
elements of components that support it.
@OvermindDL1
Copy link
Contributor

What about Menu? I've had need to attach things to both the outer container div as well as the button itself that brings up the menu. Although this will not be needed if Menu is ever converted to be like ToolTip's where you 'attach' it, but currently it is not. :-)

@debois
Copy link
Owner

debois commented Aug 25, 2016

@OvermindDL1: @aforemny is looking into (some variant of) the attach idea, see here.

@@ -99,7 +99,7 @@ type alias Config m =
, cols : Maybe Int
, autofocus : Bool
, maxlength : Maybe Int
, style : List (Options.Style m)
, inner : List (Options.Style m)
Copy link
Owner

@debois debois Aug 25, 2016

Choose a reason for hiding this comment

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

We lost style, so this needs to be merged in master, not v7, yes?

Maybe keep the name style in v7, then switch it to inner in master, add the switch to a new file MIGRATION.md in master, where we collect migration notes for the eventual v8 release.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Textfield actually does not expose Config so this is a MINOR change according to elm-package diff

@debois
Copy link
Owner

debois commented Aug 25, 2016

I made some minor line notes, but I like this. Merge it when you've settled the v7/master question.

Ordering is important to avoid problems with having users
provide important attributes with Options.inner and overriding
the ones that are necessary for proper functionality
@vipentti vipentti merged commit 2493bf9 into debois:v7 Aug 27, 2016
@vipentti vipentti deleted the id-support branch September 1, 2016 08:00
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.

3 participants