Appearance settings for compact posts

[elpendor]

• Thumbnail position (either left or right)
• Voting buttons visibility (toggles visibility for upvote/downvote buttons)

[rsammelson]

I think you should rename the show voting buttons "never" variant. It's not really never, it's not in compact mode.

Also you don't need to make a new PR with your changes, just push a new commit to your branch and this will update.

[elpendor]

@rsammelson Which name do you propose instead?

[rsammelson]

I'm not sure, maybe "not compact", others have proposed turning off the vote buttons completely in the future, which would then make sense as "never"

[elpendor]

I'm not sure I'm following your logic, what exactly do you consider out of place?

Just for clarification, this setting is to toggle visibility of the voting buttons just for the compact post view.

It's the equivalent of this setting for Apollo:

I think "never" is fitting. Perhaps what should be refactored is the variable itself: it should be something like CompactShowVotingButtons instead of ShowVotingButtons. Same thing for the thumbnail setting.

Do you agree?

[rsammelson]

Yeah that fixes the problem; at that point you could also make them booleans instead of making more types.

[elpendor]

I did it that way because that's how other booleans were handled (see OCommentThreadCollapse).

I didn't want to break the existing pattern and figured there might be a reason for it.

[elpendor]

Done.

I left CompactThumbnailPositionType as a type though.

Even if it's just two values, it does not really represent a boolean and would require pointless refactoring for it to be easily readable.

[aeharding]

Sorry for the conflicts - settings stuff is in flux!

[elpendor]

No problem, it's understandable.

I just fixed the conflicts and merged the new commits.

Also moved CompactSettings to the new AppearanceSettings.

[aeharding]

@burakcan if you have a moment can you look at this, specifically 426634f?

I'm having a problem where we add a new setting to defaultSettings and then when you get an update with new defaults, the new defaults aren't being inserted into the DB so calls to getSetting are failing.

I guess we could add add migration logic to upgrade to a new version of the DB when a new setting is added, but that doesn't really seem sustainable and then there's duplicated logic. (There's also duplicated initial settings in the redux store initial values..)

I'm wondering if we should just have getSetting return undefined if it doesn't exist? And then just don't insert defaults into the database initially?

That would also ensure we have a single source of truth for defaults: The Redux store.

Again please take a look at 426634f where I did this and let me know your thoughts. (And @elpendor you as well!)

edit: Oh and @rsammelson!

[aeharding]

(Due to no feedback yet, I'm going to go ahead and merge this as-is. Please open another PR or issue to discuss further!)

[rsammelson]

I just got a chance to look, it looks pretty much good to me. I, at least, like this approach better.

[elpendor]

Sorry, I'm out of town right now and couldn't check the code.

I agree, for now this seems like a better approach.

Haven't read the code entirely but if my PR needs to be adapted to this new pattern, I'll fix it when I get back.

[burakcan]

Hi sorry I'm also out of town so couldn't check yet. I'll try to take a look this week.

Edit: quickly took a look on mobile. I agree this makes more sense. My bad on the design of defaults.