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 value attribute to RTL checkbox #106

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@joyeusenoelle
Copy link
Collaborator

commented May 17, 2019

As described in the comments of #103.


  • I have signed the CLA
@@ -255,7 +255,7 @@
</select>
</dd>
<dt><label for="rtl">Direction</label></dt>
<dd><input type="checkbox" id="rtl" name="rtl" {{if .Post.IsRTL.Bool}}checked="checked"{{end}} /><label for="rtl"> right-to-left</label></dd>
<dd><input type="checkbox" id="rtl" name="rtl" value="true" {{if .Post.IsRTL.Bool}}checked="checked"{{end}} /><label for="rtl"> right-to-left</label></dd>

This comment has been minimized.

Copy link
@robjloranger

robjloranger May 17, 2019

Member

should this be false and unchecked by default?

This comment has been minimized.

Copy link
@joyeusenoelle

joyeusenoelle May 17, 2019

Author Collaborator

The value doesn't need to be false; if it's unchecked the element simply won't be included in POST, and we have code on the server side to handle that. The default value should reflect whatever's in the database, which this does. (It's only ever visible when a post has already been stored.)

(There is an argument to be made that we should allow these settings to be changed at the beginning of a post rather than after its first save, but that's not the state of the code at the moment.)

This comment has been minimized.

Copy link
@joyeusenoelle

joyeusenoelle May 17, 2019

Author Collaborator

There's also an argument to be made that this should be a select dropdown or a set of radio buttons, especially since the server-side code supports an "auto" setting (that changes RTL status based on the post language) that isn't available here. But I'm not sure if the "auto" code has been written, so I'm leaving that alone for now.

This comment has been minimized.

Copy link
@robjloranger

robjloranger May 17, 2019

Member

ok, I'm just thinking out loud with your help 😄

So if the value doesn't get sent if the box is not checked, how do we change the stored value when unchecking the box and submitting?

edit: nm, I got it now. thanks for clearing my confusion

@robjloranger

This comment has been minimized.

Copy link
Member

commented May 17, 2019

A quick side note, I think this PR should be branched off thedevelop branch and target the same instead of master.

@joyeusenoelle

This comment has been minimized.

Copy link
Collaborator Author

commented May 17, 2019

Oop, probably true.

@thebaer

This comment has been minimized.

Copy link
Member

commented May 18, 2019

Hey, and be sure to check out my comments in #104

@joyeusenoelle joyeusenoelle deleted the rtl-post branch May 29, 2019

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.