Skip to content

Conversation

@achingono
Copy link

My very first pull request!

First I added an condition for properties that had [ScaffoldColumn(true)]. These would be rendered so can be populated.

Next I added a condition for properties that are not ValueTypes. These would have sub-properties and the the convention for MVC is to render the control Id in the format {ParentPropertyName}_{ChildPropertyName}.

It wasn't my intention to make the pull request so big (as I understand this is frowned upon). I thought I could add a pull request for each commit... but well. Clearly, I don't know my way around Git/Github, so please feel free to guide me in the right direction.

@robdmoore
Copy link
Member

Awesome! Congrats on your first PR and thanks for lodging it against Seleno! :)

I've got some comments for your commits so I'll make them against the individual lines of code.

In general I think this is a good step and given a few changes we can work together to get it into the codebase. It's quite funny timing actually since I've been looking into sub-viewmodel support recently :)

@robdmoore
Copy link
Member

Hi @achingono,

Sorry I haven't replied yet - I was away from home at a conference the week before last and the last week I was really busy. I'll try and get around to looking at this during the week :)

@robdmoore
Copy link
Member

FYI I'm working on testing this and merging it ito the code base this weekend. Stay tuned.

@robdmoore robdmoore mentioned this pull request Dec 21, 2013
@robdmoore
Copy link
Member

I've just pulled your commits into a Pull Request (#142) that I created to add the unit tests and a bunch of other stuff.

One of the other Seleno contributors will review it and merge it.

Thanks again for your efforts - it was really awesome :)

@robdmoore robdmoore closed this Dec 21, 2013
@achingono
Copy link
Author

Thanks, very encouraging. I'll try to be more involved as I use the framework more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants