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

Add dark theme #2456

Draft
wants to merge 10 commits into
base: develop
Choose a base branch
from
Draft

Add dark theme #2456

wants to merge 10 commits into from

Conversation

grandeljay
Copy link
Contributor

In an attempt to finish what was started at #1436

I initially said I would attempt to continue where @AhmedEid3 left off but I was struggling a bit so I decided to start over. So far I have just swapped the default colors with the inverted ones but the result looks promising in the /examples/theming.html

image

Feel free to already let me know your thoughts

@lubber-de
Copy link
Member

lubber-de commented Sep 16, 2022

Please remove the /dist folder changes from your PR for easier review. Thanks

@lubber-de
Copy link
Member

@grandeljay
TL;DR: The /dist folder should not be part of your PR, not even as a delete

You have now completely deleted the /dist folder. So if we would merge it, the /dist folder gets deleted.
What i meant is to remove the /dist folder from your commit only, so the /dist folder stays untouched.
I suggest to

  • save/backup your changes in the /src folder somewhere
  • reset your branch to the develop branch (yes all changes are lost..but you kept the /src changes in the last step)
  • apply/copy your previously saved changes files to the /src folder again
  • git push --force

@grandeljay
Copy link
Contributor Author

grandeljay commented Sep 17, 2022

@lubber-de I've copied the /dist directory from the upstream/develop branch and most of the changes seem to have disappeared. Some files seem to have been added though (like flyout). Why is that? Shouldn't it all be unchanged now? Did I clone the wrong branch? It still says Version 2.8.8, shouldn't it be 2.9.0 beta or something like that?

@lubber-de
Copy link
Member

lubber-de commented Sep 17, 2022

The /dist folder from the develop branch does not contain the flyout component (yet), because the dist folder will only be updated on a new release.
But the current develop branch already has the flyout components inside the /src folder. So it seems you have build fomantic locally yourself (or did an npm install) and afterwards copied the /dist folder from there. thats why flyout is now "new" to the /dist folder in comparison to the develop branch.

Same for the /dist/themes folder. Please don't commit them

I again suggest to reset your branch to the current develop branch and only do the changes inside the /src folder.
Of course you can build locally however you like, just make sure on commit to ignore the /dist folder.

@lubber-de
Copy link
Member

It still says Version 2.8.8, shouldn't it be 2.9.0 beta or something like that?

it is still 2.8.8 inside the develop branch. 2.9.0 beta-xxxxx is set on the nightly builds only.
We'll update the version inside package.json on a new release only

@grandeljay
Copy link
Contributor Author

Ok, thanks. All the changes should be gone now.

If I remember correctly you said there is no way to make this default-dark theme work automatically with prefers-color-scheme: dark?

I'd have to create a CSS file and import it, like you mentioned here. If that's the case, maybe I shouldn't be creating the dark theme like I have now and instead go for a CSS file

@jamessampford
Copy link
Contributor

I know there are a few considerations around implementing a dark mode, but just to add my 2 cents…

Maintaining
Will both default and default-dark both be kept up to date?
Should both have a common file containing variables that related to positioning/sizing, and separate ones for colour, to ensure consistency for positioning/sizing?

Using Dark Mode
What if we want to toggle between light and dark mode manually, rather than relying on the OS setting?
Could it be that a semantic.dark.css files is generated within the default folder with just what is needed for dark? If this, then should we also have a semantic.light.css file with anything relating to anything related to anything but colour only in semantic.css? This may help with maintenance
Could, say, both prefers-color-scheme: dark as well as either body.inverted or body[data-mode=“dark”] be used? The latter could possibly also go to light mode by specifying light

@grandeljay
Copy link
Contributor Author

grandeljay commented Oct 3, 2022

Ok, I've been giving this a lot of thought.

There is no elegant, low maintenance way of doing this so far I am trying something like this:

/**
 * Dark mode
 *
 * Adds .inverted to all components.
 */
function ready() {
    const darkModePreference = window.matchMedia("(prefers-color-scheme: dark)");

    if (darkModePreference.matches) {
        const usedComponents = ["container", "grid", "button", "calendar", "card", "checkbox", "dimmer", "divider", "dropdown", "form", "header", "icon", "image", "input", "label", "list", "loader", "menu", "message", "modal", "placeholder", "popup", "progress", "segment", "sidebar", "statistics", "step", "tab", "table", "text", "toast", "api", "transition"];

        usedComponents.forEach(usedComponent => {
            let uiComponents = document.querySelectorAll('.ui.' + usedComponent);

            uiComponents.forEach(component => {
                if (component.classList.contains('inverted')) {
                    component.classList.remove('inverted');
                } else {
                    component.classList.add('inverted');
                }
            });
        });
    }
}

document.addEventListener("DOMContentLoaded", ready);

It's quite primitive but it allows me to use a dark mode without hard-coding a bunch of CSS markup. This also scales well with fomantic updates/changes.

90% looks good to me, I can do some manual tweaks in my own CSS then.

Thoughts?

@lubber-de
Copy link
Member

Nice approach! However, there are still many components, which do not have an inverted variant which possibly results into remaining visual issue when also the background is switched to something like black.

@grandeljay
Copy link
Contributor Author

grandeljay commented Oct 3, 2022

Thanks!

Would you still consider this? IMO it's the smartest solution yet. It would theoretically work with all themes!

However, there are still many components, which do not have an inverted variant

Adding missing inverted variant could be done in FUI to solve this. I think that would be the "proper" way of doing things in this case.

@jamessampford
Copy link
Contributor

Clever - could potentially also use on the docs to toggle examples between normal and inverted (fomantic/Fomantic-UI-Docs#339) this could then identify where inverted may be missing or where this may need tweaking further (such as black backgrounds for segments, as lubber pointed out, and think about what happens if a child node contains another coloured item eg. A white segment within a black one)

@lubber-de
Copy link
Member

lubber-de commented Oct 3, 2022

Would you still consider this? IMO it's the smartest solution yet. It would theoretically work with all themes!

For a proper "default dark" theme switchable feature, i believe we really have to rework all components and move many switchable color values to css variables. Then provide a core css file and 2 "colors only" default "themes" which contain color differences only. Then it is only a matter of adding/removing one single class like "dark" to the body rather than the need to iterate through the whole DOM and switching lots of classes (what happens on dynamically changed DOM nodes?)

But, for the time being and the current state of FUI themes, your approach seems valid "enough" for whom it works out nicely.

Whenever we would start telling people this is the "official" way to do it, they will start claiming what combinations will not look nice, and so on.

Adding missing inverted variant could be done in FUI to solve this. I think that would be the "proper" way of doing things in this case.

Well, it is most probably more than just adding more inverted variants. Using your approach would mean , all possible combinations have to visually work out just as nice as the current default theme only does.
And if people are using inverted variants inside their "light" theme, toggling them has to look as good as before.

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