LBRYCommunity/lbry-desktop
1
0
Fork 0
Code Issues 596 Pull requests 6 Projects Releases 238 Packages Wiki Activity

Themes #436

Closed
btzr-io wants to merge 0 commits from master into master
pull from: master
merge into: LBRYCommunity:master
Conversation 80 Commits - Files changed -
btzr-io commented 2017-08-05 03:39:38 +02:00 (Migrated from github.com)
Copy link

Update

This is merged into branch v16, which will be next release, in ad6b4f4.
https://github.com/lbryio/lbry-app/pull/436#issuecomment-327652390

Fixes

Fix for https://github.com/lbryio/lbry-app/issues/444, https://github.com/lbryio/lbry-app/issues/309

Add

Support for loading multiple themes from settings ( UI )

Todo

  • Fix conflicts.
  • Add light theme.
  • Add dark theme.
  • Load themes on daemonReady
  • Reduce themes -> one file ( non-compiled )
  • Keep current build ( clean ).
  • Update overall scss structure.
  • Make styles more flexible / customizable.
  • Fix broken styles.
  • Implement css vars
  • Remove external dependency ( dark-theme )
  • Remove compiled css

Done

  • Update dark theme -> v.2.0.0
  • Update CHANGELOG.md
  • Init load with last theme selected.
  • Set light theme as fallback.
  • Add fallback for missing theme.
  • Grab dark theme from external repo: btzr-io/lbry-dark-theme
  • Fix react warning.
  • Rename all.css > light.css
  • Update build.
  • Implement action GET_THEMES
  • Read css files from ./themes
### Update > This is merged into branch v16, which will be next release, in ad6b4f4. https://github.com/lbryio/lbry-app/pull/436#issuecomment-327652390 ### Fixes Fix for https://github.com/lbryio/lbry-app/issues/444, https://github.com/lbryio/lbry-app/issues/309 ### Add Support for loading multiple themes from settings ( UI ) ### Todo - [x] Fix conflicts. - [x] Add light theme. - [x] Add dark theme. - [x] Load themes on `daemonReady` - [x] Reduce themes -> one file ( non-compiled ) - [x] Keep current build ( clean ). - [x] Update overall scss structure. - [x] Make styles more flexible / customizable. - [x] Fix broken styles. - [x] Implement css vars - [x] Remove external dependency ( dark-theme ) - [x] Remove compiled css ### Done - [x] Update dark theme -> `v.2.0.0` - [x] Update `CHANGELOG.md` - [x] Init load with last theme selected. - [x] Set light theme as fallback. - [x] Add fallback for missing theme. - [x] Grab dark theme from external repo: [btzr-io/lbry-dark-theme](https://github.com/btzr-io/lbry-dark-theme) - [x] Fix react warning. - [X] Rename `all.css` > `light.css` - [x] Update build. - [x] Implement action `GET_THEMES` - [x] Read css files from `./themes`
kauffj (Migrated from github.com) reviewed 2017-08-05 03:39:38 +02:00
btzr-io commented 2017-08-05 04:02:00 +02:00 (Migrated from github.com)
Copy link

Is there a contributors guide / guidelines ?

Is there a contributors `guide` / `guidelines` ?
btzr-io (Migrated from github.com) reviewed 2017-08-05 05:09:09 +02:00
btzr-io (Migrated from github.com) left a comment
Copy link

Something wrong happen with 9eec9fae1a 😕 ^^

Something wrong happen with 9eec9fae1ad110739a6e4f158c71f945bc8b53d0 :confused: ^^
kauffj (Migrated from github.com) reviewed 2017-08-06 18:55:51 +02:00
kauffj (Migrated from github.com) left a comment
Copy link

Exciting stuff! Here's some initial feedback I had.

Exciting stuff! Here's some initial feedback I had.
ui/js/page/settings/view.jsx
@ -0,0 +259,4 @@
)}
/>
</div>
</section>
kauffj (Migrated from github.com) commented 2017-08-06 18:54:38 +02:00
Copy link

Please use real variable names.

Please use real variable names.
ui/js/util/getThemes.js
kauffj (Migrated from github.com) commented 2017-08-06 18:53:36 +02:00
Copy link

So I think it would be appropriate for this to handled via just selector/settings.js rather than it's own utility function.

Although technically reading from disk should be considered non-deterministic, which would mean this would need to have it's own action and reducer. But that seems somewhat over-engineered to me ( @6ea86b96 would you do that here? )

Another option would be to have the build process put the themes into app/package.json, which there is a precedence for importing configuration values from.

So I _think_ it would be appropriate for this to handled via just `selector/settings.js` rather than it's own utility function. Although technically reading from disk should be considered non-deterministic, which would mean this would need to have it's own action and reducer. But that seems somewhat over-engineered to me ( @6ea86b96 would you do that here? ) Another option would be to have the build process put the themes into `app/package.json`, which there is a precedence for importing configuration values from.
btzr-io (Migrated from github.com) reviewed 2017-08-06 19:00:43 +02:00
ui/js/page/settings/view.jsx
@ -0,0 +259,4 @@
)}
/>
</div>
</section>
btzr-io (Migrated from github.com) commented 2017-08-06 19:00:43 +02:00
Copy link

This is way the contributors guide should exist 😄 ^^

This is way the `contributors` guide should exist :smile: ^^
btzr-io (Migrated from github.com) reviewed 2017-08-06 19:16:09 +02:00
ui/js/util/getThemes.js
btzr-io (Migrated from github.com) commented 2017-08-06 19:16:09 +02:00
Copy link

@kauffj I didn't want to mess with the build...

@kauffj I didn't want to mess with the build...
kauffj (Migrated from github.com) reviewed 2017-08-07 01:37:37 +02:00
ui/js/util/getThemes.js
kauffj (Migrated from github.com) commented 2017-08-07 01:37:37 +02:00
Copy link

I'm pretty sure this can't go inside of a selector, so if we can't get it into package.json, the code for loading the themes ought to be in an action.

I'm pretty sure this can't go inside of a selector, so if we can't get it into package.json, the code for loading the themes ought to be in an action.
btzr-io (Migrated from github.com) reviewed 2017-08-07 02:02:22 +02:00
ui/js/util/getThemes.js
btzr-io (Migrated from github.com) commented 2017-08-07 02:02:22 +02:00
Copy link

ok, how do I call the action? GET_THEMES ?

ok, how do I call the action? `GET_THEMES` ?
btzr-io (Migrated from github.com) reviewed 2017-08-07 02:25:04 +02:00
ui/js/util/getThemes.js
btzr-io (Migrated from github.com) commented 2017-08-07 02:25:04 +02:00
Copy link

I'm done, I'll tidy up and fix conflicts ^^

I'm done, I'll tidy up and fix conflicts ^^
btzr-io commented 2017-08-07 03:46:50 +02:00 (Migrated from github.com)
Copy link

Fix conflicts da92061 ^^

Fix conflicts da92061 ^^
👍 1 🎉 1
btzr-io commented 2017-08-07 05:16:20 +02:00 (Migrated from github.com)
Copy link

All is left is load the dark-theme any ideas?

All is left is load the `dark-theme` any ideas?
MSFTserver commented 2017-08-07 05:47:38 +02:00 (Migrated from github.com)
Copy link

@btzr-io what do you mean by ideas? how to import it?

@btzr-io what do you mean by ideas? how to import it?
btzr-io commented 2017-08-07 05:51:52 +02:00 (Migrated from github.com)
Copy link

@MSFTserver yes ^^

@MSFTserver yes ^^
btzr-io commented 2017-08-07 05:52:07 +02:00 (Migrated from github.com)
Copy link

This my current implementation:

(
  cd "$ROOT/app/"
  yarn add https://github.com/btzr-io/lbry-dark-theme --production
  cd ./node_modules/lbry-dark-theme/dist/
  cp dark.css "$ROOT/app/dist/themes/dark.css"
)
This my current implementation: ``` ( cd "$ROOT/app/" yarn add https://github.com/btzr-io/lbry-dark-theme --production cd ./node_modules/lbry-dark-theme/dist/ cp dark.css "$ROOT/app/dist/themes/dark.css" ) ```
MSFTserver commented 2017-08-07 05:57:30 +02:00 (Migrated from github.com)
Copy link

oh i was thinking just move the file over lol

oh i was thinking just move the file over lol
btzr-io commented 2017-08-07 06:03:15 +02:00 (Migrated from github.com)
Copy link

@MSFTserver I can't because -> I'm constantly updating it ( fixing bugs ), and still experimental 😛

@MSFTserver I can't because -> I'm constantly updating it ( fixing bugs ), and still experimental :stuck_out_tongue:
MSFTserver commented 2017-08-07 06:50:27 +02:00 (Migrated from github.com)
Copy link

can there really be that many bugs in a re-theme

can there really be that many bugs in a re-theme
MSFTserver commented 2017-08-07 07:16:04 +02:00 (Migrated from github.com)
Copy link

Don't forget to write little tutorial on how to upload custom themes that may be downloaded from lbry

Don't forget to write little tutorial on how to upload custom themes that may be downloaded from lbry
👍 2
6ea86b96 (Migrated from github.com) reviewed 2017-08-07 09:33:04 +02:00
ui/js/util/getThemes.js
6ea86b96 (Migrated from github.com) commented 2017-08-07 09:33:04 +02:00
Copy link

One problem that would occur from throwing it into a selector without that selector reading from the state is that the selector will be memoized so files would be read only once. Doesn't seem like a big concern though unless people are adding themes on the fly? I would have probably gone with action/reducer/selector. Modifying the build progress also seems fine to me.

One problem that would occur from throwing it into a selector without that selector reading from the state is that the selector will be memoized so files would be read only once. Doesn't seem like a big concern though unless people are adding themes on the fly? I would have probably gone with action/reducer/selector. Modifying the build progress also seems fine to me.
btzr-io (Migrated from github.com) reviewed 2017-08-08 16:19:09 +02:00
ui/js/actions/settings.js
@ -0,0 +40,4 @@
};
}
export function doSetClientSetting(key, value) {
btzr-io (Migrated from github.com) commented 2017-08-08 16:19:05 +02:00
Copy link

Can this be just: return themes; ? ^^

Can this be just: `return themes;` ? ^^
6ea86b96 (Migrated from github.com) reviewed 2017-08-08 16:20:20 +02:00
ui/js/actions/settings.js
@ -0,0 +40,4 @@
};
}
export function doSetClientSetting(key, value) {
6ea86b96 (Migrated from github.com) commented 2017-08-08 16:20:20 +02:00
Copy link

no, actions must return this format.

no, actions must return this format.
btzr-io (Migrated from github.com) reviewed 2017-08-08 16:21:23 +02:00
ui/js/actions/settings.js
@ -0,0 +40,4 @@
};
}
export function doSetClientSetting(key, value) {
btzr-io (Migrated from github.com) commented 2017-08-08 16:21:23 +02:00
Copy link

ok thanks ^^ ( I'm new to redux 🙃 )

ok thanks ^^ ( I'm new to redux :upside_down_face: )
btzr-io commented 2017-08-12 04:12:47 +02:00 (Migrated from github.com)
Copy link

So here is my initial implementation for the fallback ( util/setThemes.js ):

import { existsSync } from 'fs';
import { remote } from 'electron';


function setTheme(name) {
  const link = document.getElementById("theme");
  const file = `${name}.css`;
  const path = `${remote.app.getAppPath()}/dist/themes/${file}`;

  if(existsSync(path)){
    link.href = `./themes/${file}`;
    lbry.setClientSetting('theme', name);
  }

  // Fallback: light theme
  else {
    link.href = `./themes/light.css`;
    lbry.setClientSetting('theme', 'light');
  }

}

export default setTheme;

Load initial theme ( ./main.js ):


...
setTheme(lbry.getClientSetting('theme'));
...

is it ok to use existsSync and check if file exist everytime a a theme is selected?
Also where do I load the initial theme, right now I'm using ./main.js
Need some feedback / review ^^

So here is my initial implementation for the fallback ( `util/setThemes.js` ): ``` JS import { existsSync } from 'fs'; import { remote } from 'electron'; function setTheme(name) { const link = document.getElementById("theme"); const file = `${name}.css`; const path = `${remote.app.getAppPath()}/dist/themes/${file}`; if(existsSync(path)){ link.href = `./themes/${file}`; lbry.setClientSetting('theme', name); } // Fallback: light theme else { link.href = `./themes/light.css`; lbry.setClientSetting('theme', 'light'); } } export default setTheme; ``` Load initial theme ( `./main.js` ): ``` JS ... setTheme(lbry.getClientSetting('theme')); ... ``` is it ok to use `existsSync` and check if file exist everytime a a theme is selected? Also where do I load the initial theme, right now I'm using `./main.js` Need some feedback / review ^^
btzr-io commented 2017-08-12 19:08:41 +02:00 (Migrated from github.com)
Copy link

Any thoughts before I push that ? ^^

Any thoughts before I push that ? ^^
kauffj (Migrated from github.com) requested changes 2017-08-12 23:09:51 +02:00
kauffj (Migrated from github.com) left a comment
Copy link

A decent number of changes will required to get this live. See inline comments below.

Additionally, I looked at your theme repo and think we will likely need a substantial set of changes to the CSS design to properly support themeing.

LBRY is still in beta, and as such things change very frequently. Element class names and SCSS set up can change at any time and new elements are added regularly.

If we're going to support themes at this time, it needs to be done in a way that both:

  • Adds zero or minimal additional development time
  • Is not fragile to changes

For both of these things to be true, I think themes need to be reduced to just a single file (or possibly two files?) containing only color and function definitions.

If we can only support themes via lots of additional CSS, I'm hesitant to add them at this time since it's pretty much guaranteed future development will break them with no effective way to detect this on a pre-emptive basis.

Feel free to discuss with me further on Slack. I hope this isn't discouraging, as this was a fantastic and substantial effort and I really like the idea.

A decent number of changes will required to get this live. See inline comments below. Additionally, I looked at your theme repo and think we will likely need a substantial set of changes to the CSS design to properly support themeing. LBRY is still in beta, and as such things change very frequently. Element class names and SCSS set up can change at any time and new elements are added regularly. If we're going to support themes at this time, it needs to be done in a way that both: - Adds zero or minimal additional development time - Is not fragile to changes For both of these things to be true, I think themes need to be reduced to just a single file (or possibly two files?) containing only color and function definitions. If we can only support themes via lots of additional CSS, I'm hesitant to add them at this time since it's pretty much guaranteed future development will break them with no effective way to detect this on a pre-emptive basis. Feel free to discuss with me further on Slack. I hope this isn't discouraging, as this was a fantastic and substantial effort and I really like the idea.
app/package.json
@ -0,0 +8,4 @@
"email": "hello@lbry.io"
},
"dependencies": {
"electron-dl": "^1.6.0",
kauffj (Migrated from github.com) commented 2017-08-12 22:42:52 +02:00
Copy link

I would prefer that we not package the themes externally.

I would prefer that we not package the themes externally.
build/build.sh
@ -0,0 +59,4 @@
############
(
cd "$ROOT/ui"
kauffj (Migrated from github.com) commented 2017-08-12 22:44:06 +02:00
Copy link

This will need to be modified to build all themes.

This will need to be modified to build all themes.
ui/dist/index.html
@ -0,0 +2,4 @@
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width">
<title>LBRY</title>
kauffj (Migrated from github.com) commented 2017-08-12 22:56:13 +02:00
Copy link

Rather than hardcoding the default here, I'd propose the following design:

  • Split out the splash screen into its own file and include here.
  • Include this tag without a href property.
  • Call setThemes before we leave the splash page.
Rather than hardcoding the default here, I'd propose the following design: - Split out the splash screen into its own file and include here. - Include this tag _without_ a `href` property. - Call `setThemes` before we leave the splash page.
ui/dist/themes/light.css
kauffj (Migrated from github.com) commented 2017-08-12 22:57:53 +02:00
Copy link

Currently no compiled CSS is in SCM. This shouldn't change.

Currently no compiled CSS is in SCM. This shouldn't change.
ui/js/actions/settings.js
@ -0,0 +40,4 @@
};
}
export function doSetClientSetting(key, value) {
kauffj (Migrated from github.com) commented 2017-08-12 23:01:55 +02:00
Copy link

@btzr-io this name is probably okay given our lack of standards re: action naming, but @6ea86b96 this makes me think we should establish a convention. We don't really have a naming pattern, nor are we consistent with our verbs (e.g. SUCCESS vs. SUCCEEDED).

I'm a a fan of noun_verb as a naming convention, since once you get used to the pattern it facilitates both deducing them and autocompleting them.

@btzr-io this name is probably okay given our lack of standards re: action naming, but @6ea86b96 this makes me think we should establish a convention. We don't really have a naming pattern, nor are we consistent with our verbs (e.g. SUCCESS vs. SUCCEEDED). I'm a a fan of `noun_verb` as a naming convention, since once you get used to the pattern it facilitates both deducing them and autocompleting them.
ui/js/constants/action_types.js
@ -0,0 +111,4 @@
export const CLAIM_REWARD_STARTED = "CLAIM_REWARD_STARTED";
export const CLAIM_REWARD_SUCCESS = "CLAIM_REWARD_SUCCESS";
export const CLAIM_REWARD_FAILURE = "CLAIM_REWARD_FAILURE";
export const CLAIM_REWARD_CLEAR_ERROR = "CLAIM_REWARD_CLEAR_ERROR";
kauffj (Migrated from github.com) commented 2017-08-12 23:02:27 +02:00
Copy link

Did you change this or did Prettier?

Did you change this or did `Prettier`?
ui/watch.sh
@ -0,0 +14,4 @@
if [ ! -d "$DIR/node_modules" ]; then
echo "Installing NPM modules"
yarn install
kauffj (Migrated from github.com) commented 2017-08-12 23:03:01 +02:00
Copy link

Just like ./build.sh, this should compile all themes.

Just like `./build.sh`, this should compile all themes.
btzr-io (Migrated from github.com) reviewed 2017-08-12 23:14:38 +02:00
ui/js/constants/action_types.js
@ -0,0 +111,4 @@
export const CLAIM_REWARD_STARTED = "CLAIM_REWARD_STARTED";
export const CLAIM_REWARD_SUCCESS = "CLAIM_REWARD_SUCCESS";
export const CLAIM_REWARD_FAILURE = "CLAIM_REWARD_FAILURE";
export const CLAIM_REWARD_CLEAR_ERROR = "CLAIM_REWARD_CLEAR_ERROR";
btzr-io (Migrated from github.com) commented 2017-08-12 23:14:37 +02:00
Copy link

Not that I remember 🤔

Not that I remember :thinking:
btzr-io (Migrated from github.com) reviewed 2017-08-12 23:17:53 +02:00
build/build.sh
@ -0,0 +59,4 @@
############
(
cd "$ROOT/ui"
btzr-io (Migrated from github.com) commented 2017-08-12 23:17:53 +02:00
Copy link

dark theme is an external module, not sure how to do it in there...

dark theme is an external module, not sure how to do it in there...
btzr-io (Migrated from github.com) reviewed 2017-08-13 00:45:18 +02:00
ui/js/actions/settings.js
@ -0,0 +40,4 @@
};
}
export function doSetClientSetting(key, value) {
btzr-io (Migrated from github.com) commented 2017-08-13 00:45:18 +02:00
Copy link

Ok, I'm still waiting that contributors guide 😛

Ok, I'm still waiting that contributors guide :stuck_out_tongue:
btzr-io (Migrated from github.com) reviewed 2017-08-13 00:45:53 +02:00
app/package.json
@ -0,0 +8,4 @@
"email": "hello@lbry.io"
},
"dependencies": {
"electron-dl": "^1.6.0",
btzr-io (Migrated from github.com) commented 2017-08-13 00:45:53 +02:00
Copy link

css vars can fix this ^^

css vars can fix this ^^
btzr-io (Migrated from github.com) reviewed 2017-08-13 00:45:59 +02:00
build/build.sh
@ -0,0 +59,4 @@
############
(
cd "$ROOT/ui"
btzr-io (Migrated from github.com) commented 2017-08-13 00:45:59 +02:00
Copy link

css vars can fix this ^^

css vars can fix this ^^
btzr-io (Migrated from github.com) reviewed 2017-08-13 00:46:19 +02:00
ui/dist/index.html
@ -0,0 +2,4 @@
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width">
<title>LBRY</title>
btzr-io (Migrated from github.com) commented 2017-08-13 00:46:19 +02:00
Copy link

no need to do that...
css vars can fix this ^^

no need to do that... css vars can fix this ^^
btzr-io (Migrated from github.com) reviewed 2017-08-13 00:46:33 +02:00
ui/dist/themes/light.css
btzr-io (Migrated from github.com) commented 2017-08-13 00:46:33 +02:00
Copy link

ok ^^

ok ^^
btzr-io (Migrated from github.com) reviewed 2017-08-13 00:47:14 +02:00
ui/js/constants/action_types.js
@ -0,0 +111,4 @@
export const CLAIM_REWARD_STARTED = "CLAIM_REWARD_STARTED";
export const CLAIM_REWARD_SUCCESS = "CLAIM_REWARD_SUCCESS";
export const CLAIM_REWARD_FAILURE = "CLAIM_REWARD_FAILURE";
export const CLAIM_REWARD_CLEAR_ERROR = "CLAIM_REWARD_CLEAR_ERROR";
btzr-io (Migrated from github.com) commented 2017-08-13 00:47:14 +02:00
Copy link

is that a problem?? ^^

is that a problem?? ^^
btzr-io (Migrated from github.com) reviewed 2017-08-13 00:48:02 +02:00
ui/watch.sh
@ -0,0 +14,4 @@
if [ ! -d "$DIR/node_modules" ]; then
echo "Installing NPM modules"
yarn install
btzr-io (Migrated from github.com) commented 2017-08-13 00:48:02 +02:00
Copy link

I can't do that, but instead we could just compile one file -> all.css

I can't do that, but instead we could just compile one file -> `all.css`
btzr-io (Migrated from github.com) reviewed 2017-08-13 00:48:41 +02:00
ui/watch.sh
@ -0,0 +14,4 @@
if [ ! -d "$DIR/node_modules" ]; then
echo "Installing NPM modules"
yarn install
btzr-io (Migrated from github.com) commented 2017-08-13 00:48:41 +02:00
Copy link

and use css vars for themes ( I know already told you but it fix everything ) 🙃

and use css vars for themes ( I know already told you but it fix everything ) :upside_down_face:
btzr-io (Migrated from github.com) reviewed 2017-08-13 00:50:30 +02:00
ui/dist/index.html
@ -0,0 +2,4 @@
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width">
<title>LBRY</title>
btzr-io (Migrated from github.com) commented 2017-08-13 00:50:30 +02:00
Copy link

Split out the splash screen into its own file and include here

> Split out the splash screen into its own file and include here
btzr-io (Migrated from github.com) reviewed 2017-08-13 00:50:45 +02:00
ui/dist/index.html
@ -0,0 +2,4 @@
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width">
<title>LBRY</title>
btzr-io (Migrated from github.com) commented 2017-08-13 00:50:45 +02:00
Copy link

You mean a component? ^^

You mean a component? ^^
btzr-io commented 2017-08-13 01:10:08 +02:00 (Migrated from github.com)
Copy link

If we can only support themes via lots of additional CSS, I'm hesitant to add them at this time since it's pretty much guaranteed future development will break them with no effective way to detect this on a pre-emptive basis.

That's why the app scss styles needs to be more flexible / customizable ^^

> If we can only support themes via lots of additional CSS, I'm hesitant to add them at this time since it's pretty much guaranteed future development will break them with no effective way to detect this on a pre-emptive basis. That's why the app `scss` styles needs to be more flexible / customizable ^^
kauffj (Migrated from github.com) reviewed 2017-08-13 01:12:13 +02:00
ui/dist/index.html
@ -0,0 +2,4 @@
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width">
<title>LBRY</title>
kauffj (Migrated from github.com) commented 2017-08-13 01:12:13 +02:00
Copy link

I mean split the splash screen CSS into its own file. That way when the app loads, the splash screen and associated CSS can display fine.

I mean split the splash screen CSS into its own file. That way when the app loads, the splash screen and associated CSS can display fine.
kauffj (Migrated from github.com) reviewed 2017-08-13 01:12:57 +02:00
build/build.sh
@ -0,0 +59,4 @@
############
(
cd "$ROOT/ui"
kauffj (Migrated from github.com) commented 2017-08-13 01:12:57 +02:00
Copy link

I'm proposing to not make it an external module but instead put the code into the project.

I'm proposing to not make it an external module but instead put the code into the project.
kauffj (Migrated from github.com) reviewed 2017-08-13 01:13:23 +02:00
ui/js/constants/action_types.js
@ -0,0 +111,4 @@
export const CLAIM_REWARD_STARTED = "CLAIM_REWARD_STARTED";
export const CLAIM_REWARD_SUCCESS = "CLAIM_REWARD_SUCCESS";
export const CLAIM_REWARD_FAILURE = "CLAIM_REWARD_FAILURE";
export const CLAIM_REWARD_CLEAR_ERROR = "CLAIM_REWARD_CLEAR_ERROR";
kauffj (Migrated from github.com) commented 2017-08-13 01:13:23 +02:00
Copy link

No problem, just leave this as-is. Was curious.

No problem, just leave this as-is. Was curious.
btzr-io commented 2017-08-13 01:13:35 +02:00 (Migrated from github.com)
Copy link

If we're going to support themes at this time, it needs to be done in a way that both:

  • Adds zero or minimal additional development time
  • Is not fragile to changes

Ok I see your point ( not so sure about the second one ) 😅

> If we're going to support themes at this time, it needs to be done in a way that both: > - Adds zero or minimal additional development time > - Is not fragile to changes Ok I see your point ( not so sure about the second one ) :sweat_smile:
kauffj commented 2017-08-13 01:15:09 +02:00 (Migrated from github.com)
Copy link

I agree that the requirement to not be fragile could be hard.

Would you be able to make the changes to the overall scss structure so that less of the additional per-theme CSS is necessary?

I agree that the requirement to not be fragile could be hard. Would you be able to make the changes to the overall scss structure so that less of the additional per-theme CSS is necessary?
👍 1
btzr-io (Migrated from github.com) reviewed 2017-08-13 01:18:49 +02:00
build/build.sh
@ -0,0 +59,4 @@
############
(
cd "$ROOT/ui"
btzr-io (Migrated from github.com) commented 2017-08-13 01:18:48 +02:00
Copy link

I mean no need to move all the code, since we can reduce it to one file ^^

I mean no need to move all the code, since we can reduce it to one file ^^
kauffj (Migrated from github.com) reviewed 2017-08-13 01:21:48 +02:00
build/build.sh
@ -0,0 +59,4 @@
############
(
cd "$ROOT/ui"
kauffj (Migrated from github.com) commented 2017-08-13 01:21:48 +02:00
Copy link

Gotcha.

Gotcha.
btzr-io commented 2017-08-13 01:26:07 +02:00 (Migrated from github.com)
Copy link

Well you can close this (at least for now)...

Well you can close this (at least for now)...
btzr-io (Migrated from github.com) reviewed 2017-08-13 07:04:43 +02:00
ui/js/actions/settings.js
@ -0,0 +40,4 @@
};
}
export function doSetClientSetting(key, value) {
btzr-io (Migrated from github.com) commented 2017-08-13 07:04:43 +02:00
Copy link

so THEMES_GET ? ^^

so `THEMES_GET` ? ^^
btzr-io commented 2017-08-18 06:45:48 +02:00 (Migrated from github.com)
Copy link

fix conflicts ba35ac8

fix conflicts ba35ac8
btzr-io commented 2017-08-19 00:48:06 +02:00 (Migrated from github.com)
Copy link

I can't find where this var is used: $mobile-width-threshold
https://github.com/lbryio/lbry-app/blob/master/ui/scss/_global.scss#L27

Is it safe to remove it? ^^

I can't find where this var is used: `$mobile-width-threshold` https://github.com/lbryio/lbry-app/blob/master/ui/scss/_global.scss#L27 Is it safe to remove it? ^^
btzr-io commented 2017-08-19 06:41:22 +02:00 (Migrated from github.com)
Copy link

So after a long and boring week, finally I made some progress.
Before I continue to work on this can someone review it? ^^

Once this scss changes are approved I can finish the theme system
already spent more than week in this PR 😛
otherwise this should be closed.

So after a long and boring week, finally I made some progress. Before I continue to work on this can someone review it? ^^ Once this `scss` changes are approved I can finish the theme system already spent more than week in this `PR` :stuck_out_tongue: otherwise this should be closed.
kauffj (Migrated from github.com) requested changes 2017-08-19 20:24:04 +02:00
kauffj (Migrated from github.com) left a comment
Copy link

Next round of feedback :) Good progress!

Next round of feedback :) Good progress!
CHANGELOG.md
@ -19,2 +5,3 @@
## [0.53.7] - [2022-11-10]
The LBRY Web UI comes bundled as part of [LBRYApp](https://github.com/lbryio/lbry-app).
Web UI version numbers should always match the corresponding version of LBRY App.
kauffj (Migrated from github.com) commented 2017-08-19 20:19:55 +02:00
Copy link

Looks like this got duplicated

Looks like this got duplicated
ui/js/actions/settings.js
@ -0,0 +40,4 @@
};
}
export function doSetClientSetting(key, value) {
kauffj (Migrated from github.com) commented 2017-08-19 20:20:26 +02:00
Copy link

Looks like this still needs to be refactored.

Looks like this still needs to be refactored.
ui/js/main.js
@ -0,0 +4,4 @@
import App from "component/app/index.js";
import SnackBar from "component/snackBar";
import { Provider } from "react-redux";
import store from "store.js";
kauffj (Migrated from github.com) commented 2017-08-19 20:21:12 +02:00
Copy link

This also needs to be refactored. This needs to be done react/redux style, and should fit into existing settings selectors/reducers/actions.

This also needs to be refactored. This needs to be done react/redux style, and should fit into existing settings selectors/reducers/actions.
ui/js/page/settings/view.jsx
@ -0,0 +17,4 @@
this.state = {
// isMaxUpload: daemonSettings && daemonSettings.max_upload != 0,
// isMaxDownload: daemonSettings && daemonSettings.max_download != 0,
showUnavailable: lbry.getClientSetting(settings.SHOW_UNAVAILABLE),
kauffj (Migrated from github.com) commented 2017-08-19 20:21:37 +02:00
Copy link

Please add this to constants/settings.js (this is new).

Please add this to `constants/settings.js` (this is new).
ui/js/util/setTheme.js
kauffj (Migrated from github.com) commented 2017-08-19 20:22:01 +02:00
Copy link

???

???
ui/scss/component/_table.scss
@ -23,0 +31,4 @@
tfoot td {
padding: $spacing-vertical / 2 8px;
font-size: .85em;
}
kauffj (Migrated from github.com) commented 2017-08-19 20:22:26 +02:00
Copy link

Typo (ever instead of even)

Typo (ever instead of even)
ui/scss/component/_tooltip.scss
@ -6,1 +15,4 @@
margin-left: calc(var(--tooltip-width) * -1 / 2);
white-space: normal;
box-sizing: border-box;
padding: $spacing-vertical / 2;
kauffj (Migrated from github.com) commented 2017-08-19 20:23:47 +02:00
Copy link

Is tooltip-width defined? Is calc necessary?

Is tooltip-width defined? Is calc necessary?
btzr-io (Migrated from github.com) reviewed 2017-08-19 20:26:16 +02:00
ui/js/main.js
@ -0,0 +4,4 @@
import App from "component/app/index.js";
import SnackBar from "component/snackBar";
import { Provider } from "react-redux";
import store from "store.js";
btzr-io (Migrated from github.com) commented 2017-08-19 20:26:16 +02:00
Copy link

yes, I didn't work on that, just in scss

yes, I didn't work on that, just in `scss`
btzr-io (Migrated from github.com) reviewed 2017-08-19 20:27:51 +02:00
ui/scss/component/_tooltip.scss
@ -6,1 +15,4 @@
margin-left: calc(var(--tooltip-width) * -1 / 2);
white-space: normal;
box-sizing: border-box;
padding: $spacing-vertical / 2;
btzr-io (Migrated from github.com) commented 2017-08-19 20:27:51 +02:00
Copy link

let me check...

let me check...
btzr-io (Migrated from github.com) reviewed 2017-08-19 20:29:44 +02:00
ui/scss/component/_table.scss
@ -23,0 +31,4 @@
tfoot td {
padding: $spacing-vertical / 2 8px;
font-size: .85em;
}
btzr-io (Migrated from github.com) commented 2017-08-19 20:29:44 +02:00
Copy link

oh didn't notice, thanks

oh didn't notice, thanks
btzr-io (Migrated from github.com) reviewed 2017-08-19 20:30:27 +02:00
ui/js/util/setTheme.js
btzr-io (Migrated from github.com) commented 2017-08-19 20:30:27 +02:00
Copy link

yeah I'm gonna remove that, I just update scss

yeah I'm gonna remove that, I just update `scss`
btzr-io (Migrated from github.com) reviewed 2017-08-20 17:08:16 +02:00
ui/scss/component/_tooltip.scss
@ -6,1 +15,4 @@
margin-left: calc(var(--tooltip-width) * -1 / 2);
white-space: normal;
box-sizing: border-box;
padding: $spacing-vertical / 2;
btzr-io (Migrated from github.com) commented 2017-08-20 17:08:16 +02:00
Copy link

yeah, now it's defined ^^

yeah, now it's defined ^^
btzr-io (Migrated from github.com) reviewed 2017-08-20 17:10:12 +02:00
ui/scss/component/_tooltip.scss
@ -6,1 +15,4 @@
margin-left: calc(var(--tooltip-width) * -1 / 2);
white-space: normal;
box-sizing: border-box;
padding: $spacing-vertical / 2;
btzr-io (Migrated from github.com) commented 2017-08-20 17:10:12 +02:00
Copy link

4ddecf2aaa/ui/scss/_vars.scss (L115)

https://github.com/btzr-io/lbry-app/blob/4ddecf2aaa87b720d664c2f1ab3d9a8f2b07a929/ui/scss/_vars.scss#L115
btzr-io (Migrated from github.com) reviewed 2017-08-20 17:12:43 +02:00
ui/scss/component/_table.scss
@ -23,0 +31,4 @@
tfoot td {
padding: $spacing-vertical / 2 8px;
font-size: .85em;
}
btzr-io (Migrated from github.com) commented 2017-08-20 17:12:43 +02:00
Copy link

https://github.com/btzr-io/lbry-app/blob/master/ui/scss/component/_table.scss#L41

https://github.com/btzr-io/lbry-app/blob/master/ui/scss/component/_table.scss#L41
btzr-io (Migrated from github.com) reviewed 2017-08-20 17:12:53 +02:00
ui/scss/component/_table.scss
@ -23,0 +31,4 @@
tfoot td {
padding: $spacing-vertical / 2 8px;
font-size: .85em;
}
btzr-io (Migrated from github.com) commented 2017-08-20 17:12:53 +02:00
Copy link

fixed ^^

fixed ^^
btzr-io (Migrated from github.com) reviewed 2017-08-20 17:13:50 +02:00
ui/js/util/setTheme.js
btzr-io (Migrated from github.com) commented 2017-08-20 17:13:50 +02:00
Copy link

fixed -> 58496c1

fixed -> 58496c1
btzr-io (Migrated from github.com) reviewed 2017-08-20 17:25:24 +02:00
ui/scss/component/_tooltip.scss
@ -6,1 +15,4 @@
margin-left: calc(var(--tooltip-width) * -1 / 2);
white-space: normal;
box-sizing: border-box;
padding: $spacing-vertical / 2;
btzr-io (Migrated from github.com) commented 2017-08-20 17:25:24 +02:00
Copy link

here ^^

here ^^
btzr-io (Migrated from github.com) reviewed 2017-08-20 17:27:11 +02:00
ui/js/page/settings/view.jsx
@ -0,0 +17,4 @@
this.state = {
// isMaxUpload: daemonSettings && daemonSettings.max_upload != 0,
// isMaxDownload: daemonSettings && daemonSettings.max_download != 0,
showUnavailable: lbry.getClientSetting(settings.SHOW_UNAVAILABLE),
btzr-io (Migrated from github.com) commented 2017-08-20 17:27:11 +02:00
Copy link

https://github.com/btzr-io/lbry-app/blob/master/ui/js/constants/settings.js#L6

https://github.com/btzr-io/lbry-app/blob/master/ui/js/constants/settings.js#L6
btzr-io (Migrated from github.com) reviewed 2017-08-20 17:27:56 +02:00
ui/js/page/settings/view.jsx
@ -0,0 +17,4 @@
this.state = {
// isMaxUpload: daemonSettings && daemonSettings.max_upload != 0,
// isMaxDownload: daemonSettings && daemonSettings.max_download != 0,
showUnavailable: lbry.getClientSetting(settings.SHOW_UNAVAILABLE),
btzr-io (Migrated from github.com) commented 2017-08-20 17:27:56 +02:00
Copy link

fixed ^^

fixed ^^
btzr-io (Migrated from github.com) reviewed 2017-08-20 17:28:48 +02:00
ui/js/main.js
@ -0,0 +4,4 @@
import App from "component/app/index.js";
import SnackBar from "component/snackBar";
import { Provider } from "react-redux";
import store from "store.js";
btzr-io (Migrated from github.com) commented 2017-08-20 17:28:48 +02:00
Copy link

https://github.com/btzr-io/lbry-app/blob/master/ui/js/actions/settings.js#L46

https://github.com/btzr-io/lbry-app/blob/master/ui/js/actions/settings.js#L46
btzr-io (Migrated from github.com) reviewed 2017-08-20 17:29:23 +02:00
ui/js/main.js
@ -0,0 +4,4 @@
import App from "component/app/index.js";
import SnackBar from "component/snackBar";
import { Provider } from "react-redux";
import store from "store.js";
btzr-io (Migrated from github.com) commented 2017-08-20 17:29:23 +02:00
Copy link

Here is my first try 🙃 ^^

Here is my first try :upside_down_face: ^^
btzr-io (Migrated from github.com) reviewed 2017-08-20 17:31:19 +02:00
CHANGELOG.md
@ -19,2 +5,3 @@
## [0.53.7] - [2022-11-10]
The LBRY Web UI comes bundled as part of [LBRYApp](https://github.com/lbryio/lbry-app).
Web UI version numbers should always match the corresponding version of LBRY App.
btzr-io (Migrated from github.com) commented 2017-08-20 17:31:19 +02:00
Copy link

https://github.com/btzr-io/lbry-app/blob/master/CHANGELOG.md

https://github.com/btzr-io/lbry-app/blob/master/CHANGELOG.md
btzr-io commented 2017-08-20 17:49:01 +02:00 (Migrated from github.com)
Copy link

Quick fix:

  • Dark theme it's broken, fix styles.
### Quick fix: - [x] Dark theme it's broken, fix styles.
btzr-io commented 2017-08-22 04:49:14 +02:00 (Migrated from github.com)
Copy link

@kauffj I'm ready for the third round 🙃 ^^

@kauffj I'm ready for the third round :upside_down_face: ^^
btzr-io (Migrated from github.com) reviewed 2017-08-22 04:56:45 +02:00
ui/js/component/app/view.jsx
@ -0,0 +7,4 @@
class App extends React.PureComponent {
componentWillMount() {
const {
alertError,
btzr-io (Migrated from github.com) commented 2017-08-22 04:56:29 +02:00
Copy link

@kauffj Would be ok to move this -> splash component? ^^

@kauffj Would be ok to move this -> `splash` component? ^^
kauffj (Migrated from github.com) requested changes 2017-08-23 00:37:30 +02:00
kauffj (Migrated from github.com) left a comment
Copy link

Inspection only on this review, but looks super close!

Inspection only on this review, but looks super close!
ui/js/actions/app.js
@ -0,0 +347,4 @@
return Promise.resolve();
};
}
kauffj (Migrated from github.com) commented 2017-08-22 23:51:43 +02:00
Copy link

settings.THEMES

`settings.THEMES`
ui/js/component/app/view.jsx
@ -0,0 +7,4 @@
class App extends React.PureComponent {
componentWillMount() {
const {
alertError,
kauffj (Migrated from github.com) commented 2017-08-23 00:32:17 +02:00
Copy link

I think getThemes would only need to be called by the willComponentMount method in page/settings and we should use a design that doesn't require a call to setTheme at all.

I think getThemes would only need to be called by the `willComponentMount` method in `page/settings` and we should use a design that doesn't require a call to `setTheme` at all.
kauffj (Migrated from github.com) commented 2017-08-23 00:36:59 +02:00
Copy link

Put setTheme in doDaemonReady if we need to keep it.

Put `setTheme` in `doDaemonReady` if we need to keep it.
kauffj (Migrated from github.com) commented 2017-08-23 00:37:16 +02:00
Copy link

(And shouldn't this be setting the theme to the current setting value?)

(And shouldn't this be setting the theme to the current setting value?)
btzr-io (Migrated from github.com) reviewed 2017-08-24 17:20:17 +02:00
ui/js/component/app/view.jsx
@ -0,0 +7,4 @@
class App extends React.PureComponent {
componentWillMount() {
const {
alertError,
btzr-io (Migrated from github.com) commented 2017-08-24 17:20:17 +02:00
Copy link

it has a fallback to do that: https://github.com/btzr-io/lbry-app/blob/master/ui/js/actions/settings.js#L55

it has a fallback to do that: https://github.com/btzr-io/lbry-app/blob/master/ui/js/actions/settings.js#L55
btzr-io (Migrated from github.com) reviewed 2017-08-24 18:18:22 +02:00
ui/js/component/app/view.jsx
@ -0,0 +7,4 @@
class App extends React.PureComponent {
componentWillMount() {
const {
alertError,
btzr-io (Migrated from github.com) commented 2017-08-24 18:18:22 +02:00
Copy link

ok, done 👍

ok, done :+1:
btzr-io (Migrated from github.com) reviewed 2017-08-24 18:18:58 +02:00
ui/js/actions/app.js
@ -0,0 +347,4 @@
return Promise.resolve();
};
}
btzr-io (Migrated from github.com) commented 2017-08-24 18:18:58 +02:00
Copy link

fixed: https://github.com/lbryio/lbry-app/pull/436/commits/8b8c9c9da935fd47052fc8d6b3635d48dd3a4fa7

fixed: https://github.com/lbryio/lbry-app/pull/436/commits/8b8c9c9da935fd47052fc8d6b3635d48dd3a4fa7
btzr-io commented 2017-08-24 18:34:03 +02:00 (Migrated from github.com)
Copy link

@kauffj fixed last review, so I'll wait for the next one ^^ 🙃

@kauffj fixed last review, so I'll wait for the next one ^^ :upside_down_face:
btzr-io (Migrated from github.com) reviewed 2017-08-24 18:37:14 +02:00
ui/js/actions/settings.js
@ -0,0 +43,4 @@
export function doSetClientSetting(key, value) {
lbry.setClientSetting(key, value);
return {
btzr-io (Migrated from github.com) commented 2017-08-24 18:37:07 +02:00
Copy link

Yeah, probably not the best implementation, any ideas ?

Yeah, probably not the best implementation, any ideas ?
btzr-io (Migrated from github.com) reviewed 2017-08-24 18:58:58 +02:00
ui/js/actions/app.js
@ -0,0 +317,4 @@
`${path}`
);
dispatch(doAuthenticate());
dispatch({
btzr-io (Migrated from github.com) commented 2017-08-24 18:58:55 +02:00
Copy link

@kauffj ^^

@kauffj ^^
btzr-io commented 2017-08-24 20:29:07 +02:00 (Migrated from github.com)
Copy link

Fix conflicts 03b04ad

Fix conflicts 03b04ad
btzr-io commented 2017-08-25 05:54:29 +02:00 (Migrated from github.com)
Copy link

Is this going on the right direction 😕 ? ^^

Is this going on the right direction :confused: ? ^^
btzr-io commented 2017-08-31 17:42:16 +02:00 (Migrated from github.com)
Copy link

Just waiting for the new release...

Just waiting for the new release...
kauffj commented 2017-09-07 02:59:11 +02:00 (Migrated from github.com)
Copy link

This is merged into branch v16, which will be next release, in ad6b4f4f03.

See 1019a94221 for my final changes, which were:

  • Refactor theme from head tag into a component
  • Eliminate need of a special action for setting theme
  • Reduction of state data stored

This is a big win in terms of CSS refactoring and adopting the best standards, in addition to the ability to give users greater flexibility in aesthetics.

This is merged into branch `v16`, which will be next release, in https://github.com/lbryio/lbry-app/commit/ad6b4f4f03079625539306b78511df1d0141c750. See https://github.com/lbryio/lbry-app/commit/1019a942216ac0c878132e61d59a75a1d7543f9b for my final changes, which were: - Refactor theme from head tag into a component - Eliminate need of a special action for setting theme - Reduction of state data stored This is a big win in terms of CSS refactoring and adopting the best standards, in addition to the ability to give users greater flexibility in aesthetics.
👍 1 🎉 1

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
kauffj
Labels
Clear labels   
accessibility

   
app-parity

   
area: creator

   
area: daemon

   
area: design

   
area: devops

   
area: discovery

   
area: docs

   
area: installer

   
area: internal

   
area: livestream

   
area: performance

   
area: proposal

Work is part of a proposal

  
area: reposts

   
area: rewards

   
area: search

   
area: security

   
area: subscriptions

   
area: sync

   
area: ux

   
area: viewer

   
area: wallet

   
BEAMER

Beamer is waiting on you!

  
channel

   
comments

   
community PR

   
consider soon

Discuss this issue at the next planning meeting, then remove this label

  
core team

   
css

   
dependencies

   
electron

   
Epic

   
feature request

   
first-timers-only

   
good first issue

   
hacktoberfest

Welcome to Hacktoberfest

  
help wanted

   
hub-dependent

   
icebox

Long-term storage

  
Invalid

   
level: 0

   
level: 1

No knowledge of the existing code required

  
level: 2

Some knowledge of the existing code is recommended

  
level: 3

Significant knowledge of the existing code is recommended

  
level: 4

Intimate knowledge of the existing code is recommended

  
merge when green

   
needs: exploration

Solution unclear, needs research

  
needs: grooming

Issue needs to be groomed before work can start

  
needs: priority

Priority level needs to be defined

  
needs: repro

Issue needs step-by-step instructions on how to reproduce in latest code

  
needs: tech design

Needs technical design signoff before implementation can begin

  
notifications

   
odysee

   
on hold

Temporarily paused

  
playlists

   
priority: blocker

Issue is blocking release, do ASAP

  
priority: high

Work needs to be moved into sprint ASAP

  
priority: low

Work should be done but can stay in the backlog for now

  
priority: medium

Work needs to be done within 2-3 sprints

  
protocol dependent

   
recsys

   
redesign

general redesign not prioritize for anyone release

  
regression

   
resilience

   
sdk dependent

Requires work on lbry-sdk repo

  
Tom's Wishlist

   
trending

   
type: bug

Existing functionality is wrong or broken

  
type: discussion

A conversation. No specific changes requested

  
type: improvement

Existing (or partially existing) functionality needs to be changed

  
type: new feature

New functionality that does not exist yet

  
type: refactor

Minimal user-visible changes, but significant internal work

  
type: task

Either work that's not related to the code, or a small chore that does not fit into other categories

  
type: testing

Solution needs additional user testing

  
unplanned

Work that was not planned into the spirnt, took priority over planned work

  
windows
No labels
accessibility
app-parity
area: creator
area: daemon
area: design
area: devops
area: discovery
area: docs
area: installer
area: internal
area: livestream
area: performance
area: proposal
area: reposts
area: rewards
area: search
area: security
area: subscriptions
area: sync
area: ux
area: viewer
area: wallet
BEAMER
channel
comments
community PR
consider soon
core team
css
dependencies
electron
Epic
feature request
first-timers-only
good first issue
hacktoberfest
help wanted
hub-dependent
icebox
Invalid
level: 0
level: 1
level: 2
level: 3
level: 4
merge when green
needs: exploration
needs: grooming
needs: priority
needs: repro
needs: tech design
notifications
odysee
on hold
playlists
priority: blocker
priority: high
priority: low
priority: medium
protocol dependent
recsys
redesign
regression
resilience
sdk dependent
Tom's Wishlist
trending
type: bug
type: discussion
type: improvement
type: new feature
type: refactor
type: task
type: testing
unplanned
windows
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbry-desktop#436
No description provided.
Delete branch "master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?