Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

Theming#399 #402

Merged
merged 5 commits into from
Nov 16, 2017
Merged

Theming#399 #402

merged 5 commits into from
Nov 16, 2017

Conversation

aghArdeshir
Copy link

HI! This fixes issue #399 + minor theming tweaks

@j-a-m-l j-a-m-l self-requested a review November 16, 2017 10:50
Copy link
Contributor

@j-a-m-l j-a-m-l left a comment

Choose a reason for hiding this comment

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

When I go to Appearance > Theme I can't see the themes.

I see an error on the console (you can open it with Ctrl+D): ReferenceError: vShare is not defined

@@ -219,7 +219,7 @@
if (!self.network.themeDark) self.network.themeDark = false

// will be used in view
self.currentTheme = self.network.theme
self.currentTheme = 'default';//self.network.theme

Choose a reason for hiding this comment

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

Should probably remove the commented out code here

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @spresnal . But setting theme's name before registering it caused a lot of extra watches. watching themes and colors in angularJS Material is so expensive and inefficient
screenshot from 2017-11-16 23-08-42
BTW, Thanks for your hint, I made some other changes, if they didn't fix it, I'll revert them. (just preserving the main fix commit)

THanks :)

@aghArdeshir
Copy link
Author

@j-a-m-l Thanks for your review. I actually couldn't find out what the error you told me was about : ReferenceError: vShare is not defined
that vShare I think is an iOS thing. I'm not sure what that was about and I couldn't reproduce it, but yeah, my commits caused more issues. I added two more commits. Please check if what you did happens with these new commits.
Or if possible please tell me how to reproduce the issue . Thanks : )

BTW,that vShare .. were you thinking `bout something else while writing that review ? : )

Copy link
Contributor

@j-a-m-l j-a-m-l left a comment

Choose a reason for hiding this comment

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

The vShare problem was exclusive of my environment: my app/dependencies.min.js wasn't correctly generated by npm run bundle.

@j-a-m-l j-a-m-l merged commit b6431d7 into ArkEcosystem:master Nov 16, 2017
@aghArdeshir aghArdeshir deleted the theming#399 branch November 17, 2017 17:19
@fix
Copy link

fix commented Nov 18, 2017

i have the vshare issue here with latest from master, on macOs

@fix
Copy link

fix commented Nov 18, 2017

@j-a-m-l did you fix the vshare issue?

@aghArdeshir
Copy link
Author

@fix I couldn't reproduce issue. So I, no! Didn't

@j-a-m-l
Copy link
Contributor

j-a-m-l commented Nov 25, 2017

I haven't had that vShare problem again.

@boldninja
Copy link
Member

@Ardeshir81 can you provide me with your ARK address?

@aghArdeshir
Copy link
Author

@boldninja yeah . can I ask why ? do you want it here ? or somewhere else ?

@boldninja
Copy link
Member

@Ardeshir81 you can contact me on Slack if you don't want it public. Its to receive github bounty.

@aghArdeshir
Copy link
Author

Thank you @boldninja . I'll contact you in slack

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants