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

added filter invert to images #2599

Merged
merged 5 commits into from
Mar 2, 2020
Merged

added filter invert to images #2599

merged 5 commits into from
Mar 2, 2020

Conversation

LianaHus
Copy link
Collaborator

@LianaHus LianaHus commented Feb 13, 2020

@LianaHus LianaHus force-pushed the themeImgs branch 2 times, most recently from 2d82717 to e115f67 Compare February 21, 2020 12:32
@@ -26,7 +26,7 @@ const themes = [
const profile = {
name: 'theme',
events: ['themeChanged'],
methods: ['switchTheme', 'getThemes', 'currentTheme'],
methods: ['switchTheme', 'getThemes', 'currentTheme', 'fixInvert'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

fixInvert looks like a method internal to Remix and should not be called by other plugins.
btw switchTheme neither I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made it external in purpose. why not? Sokrates, for example, could use it for their Logo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah but that is more an helper that could be made available through the remix plugin than an API function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what do you mean by "remix plugin" ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant the remix-plugin library

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

some images are in color and some shouldn't change the invert.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and if I pass id it will work. but does it make sense at all? do you think it worth to have such a function? Is it obvious that invert can be used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it doesn't make so much sense to have an "invert" function. It would make more sense to provide a list of classes we are using in the application for developers to create their own theme.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok then i'll remove it

@yann300
Copy link
Collaborator

yann300 commented Feb 21, 2020

could you remove the change in package-lock.json?

@LianaHus
Copy link
Collaborator Author

could you remove the change in package-lock.json?

I did

img.style.filter = `invert(${invert})`
}
globalRegistry.get('themeModule').api.events.on('themeChanged', () => {
globalRegistry.get('themeModule').api.fixInvert(document.getElementById('swarmLogo'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't need that for the ipfs logo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought about it but it has color.. not sure. do you want me to add?

Copy link
Collaborator

Choose a reason for hiding this comment

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

do you want me to add?

not necessarily, just wanted to make sure you were aware of that

@yann300 yann300 self-requested a review March 2, 2020 16:05
@yann300 yann300 merged commit 57ce7ab into master Mar 2, 2020
@yann300 yann300 deleted the themeImgs branch March 2, 2020 16:16
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.

3 participants