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

Select background #1001

Merged
merged 7 commits into from
Sep 26, 2019
Merged

Select background #1001

merged 7 commits into from
Sep 26, 2019

Conversation

Simon-Laux
Copy link
Member

@Simon-Laux Simon-Laux commented Sep 23, 2019

  • Logic
  • Styling
  • Translation Strings
    2019-09-23_17-35

@Simon-Laux
Copy link
Member Author

Simon-Laux commented Sep 23, 2019

Ah we still need to decide on the default background on all platforms for next release @r10s @Jikstra
If we put in the new one("potato-stamps") on android too or if we keep the old one and decide at the next release.
It would bring some visual change for users to see. (looks at all under the hood changes and updates we're making with the rust core)

@r10s
Copy link
Member

r10s commented Sep 23, 2019

tbh, i would prefer a decision on that not now. as said, the potato-stamp is not my favorite, but when everyone likes it, we can go for it ;) but it could be technically better, though.

but maybe we can also use different backgrounds in the upcoming release and sync them later. wfiw, ios currently has no background at all.

@Simon-Laux Simon-Laux changed the title 🚧 Select background Select background Sep 23, 2019
@Simon-Laux Simon-Laux requested review from Jikstra, nicodh and ralphtheninja and removed request for Jikstra and nicodh September 23, 2019 16:03
Copy link
Member

@ralphtheninja ralphtheninja left a comment

Choose a reason for hiding this comment

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

Just some opinions. Tweak if you like.

const { saved } = app.state
saved[key] = value
app.saveState({ saved })
render()
}
ipcMain.on('updateDesktopSetting', updateDesktopSetting)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe do

 ipcMain.on('updateDesktopSetting', (e, k, v) => updateDesktopSetting(k, v))

Copy link
Member Author

Choose a reason for hiding this comment

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

a new function... maybe more clearer, but us the definition is above it should be fine

@@ -173,11 +173,34 @@ function init (cwd, state, logHandler) {
e.returnValue = app.localeData
})

ipcMain.on('updateDesktopSetting', (e, key, value) => {
const updateDesktopSetting = (e, key, value) => {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove e since we don't use it?

log.error('BG-IMG Copy Failed', err)
return
}
updateDesktopSetting(null, 'chatViewBgImg', `url("${newPath}")`)
Copy link
Member

Choose a reason for hiding this comment

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

.. and if so, remove null here :)

@OzancanKaratas
Copy link
Contributor

I saw some typos. However, I cannot create pull request because commits are conflicting. Please manually merge this commit.

@ralphtheninja
Copy link
Member

ralphtheninja commented Sep 23, 2019

I saw some typos. However, I cannot create pull request because commits are conflicting. Please manually merge this commit.

@OzancanKaratas Why are commits conflicting? It should work fine if you rebase your work on latest master or on this branch?

@Simon-Laux
Copy link
Member Author

@OzancanKaratas please make a new pr for this, It's not changing many typos that were introduced in this pr from what I can see with my sleepy eyes. Also the translations get also fixed on transifex sooner or later so I don't even know if it's worth the effort to do it right now.

/>
</ChatViewWrapper>
<SettingsContext.Consumer>
{(settings) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use hooks here?

@Simon-Laux Simon-Laux merged commit 5311662 into master Sep 26, 2019
@ralphtheninja ralphtheninja deleted the select_background branch September 26, 2019 19:51
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.

5 participants