Code review comment for lp:~acoconut/systers/flexible_essays

Revision history for this message
Ana Cutillas (acoconut) wrote :

On Thu, Jun 28, 2012 at 6:51 AM, Robin J <email address hidden> wrote:

> OK, we are definitely making progress. Some questions and one change
> request:
> - what do fields 3 and 4 mean in your data structure (0, and 'bottom'
> for the checkbox?)
>

I am not really doing anything with them yet. They are there for when I
start adding options to make the forms beautiful and let the admin move the
boxes around. Field 3 is the size of the box, a checkbox would look the
same with size 3000 or 0, so we can leave it to 0. Field 4 is the relative
position, I haven't really given it a lot of thought yet, I am currently
printing them in the order they appear in the list. Do you think it would
be better if I took those fields out of the data structure for now if I'm
not using them yet?

  - I'm not sure what you are doing for the checkbox (because I don't
> really understand that html). The part that confuses me is that you are
> using question[1] there twice. One part I think you are naming the
> checkbox, but why the second time? It seems to also be the value?
>
> According to what I read on the internet, there are two fields in the html
syntax to add checkboxes: name and value. Name is just the name of the
checkbox, and value is the value that is submitted if the checkbox is
checked. That's why I'm using the same string for both.

So the HTML code for a checkbox would look like this:

<TD><input type='checkbox' name='woman' value='woman'></TD>

> My requested change is that questions is too similar to mlist.questions.
> I think the code will be much easier for someone else to read if you
> change that to something like html_questions
>

Ok, changed! Let me know what you think about the extra 3 and 4 fields so I
can either change that or resubmit this again.

> --
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/112331
> You are the owner of lp:~acoconut/systers/flexible_essays.
>

« Back to merge proposal