-
Notifications
You must be signed in to change notification settings - Fork 25
Feat - url for dark version of placeholder images #540
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good idea to add a property for the dark theme like that. I prefer if we could get this information via a React hook config or css directly. I suppose, you already check the second choice. Check with @econdepe how to know if dark is applied.
Yes, we already had this discussion with @econdepe, this rely on CSS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with this for now and we can think of how to improve it later
We introduced a dummy class in in the PR #537 for an unrelated issue (Nico needed it in the subscription flow). Since the class is there, it does make sense to re-use it as in this PR, but I agree a React hook would look nicer. |
I think this suggestion looks slightly better than this PR though: // somewhere
export const getLightOrDark = (light, dark) => {
return document.body.classList.contains('isDarkMode') ? dark : light;
};
// elsewhere
<IllustrationPlaceholder
url={getDarkOrLight(lighImageImport, darkImageImport)}
...
/> I think it's as fast as doing this, and then we can refactor with the ThemeProvider later |
9d3b3b5
to
6294701
Compare
6294701
to
8494de9
Compare
Sorry about the force-pushes (the first one was unintentional...). The helper is available in ProtonMail/proton-shared#114 |
Short description of what this resolves:
Missing support of dark mode version for placeholder illustrations.
Changes proposed in this pull request:
urlDarkVersion
property (not mandatory, in some cases, the image could fit in both cases ^^)hide-on-darkmode
in case of dark version (to hide the "light" version)Snapshot