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

chore: add css custom properties for explore page component #471

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kaf-lamed-beyt
Copy link

This issue #1702 needs dark mode implemented on ipfs-webui, and this PR already addressed part of it.

Since the Explore page on ipfs-webui relies on components from this library, the component in question is expected to include themed-focus styles.

@kaf-lamed-beyt kaf-lamed-beyt requested a review from a team as a code owner March 1, 2025 23:18
Copy link

welcome bot commented Mar 1, 2025

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

  • The context for this PR is clear, with relevant discussion, decisions
    and stakeholders linked/mentioned.

  • Your contribution itself is clear (code comments, self-review for the
    rest) and in its best form. Follow the code contribution
    guidelines

    if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

  • A maintainer will triage and assign priority to this PR, commenting on
    any missing things and potentially assigning a reviewer for high
    priority items.

  • The PR gets reviews, discussed and approvals as needed.

  • The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

@SgtPooki
Copy link
Member

SgtPooki commented Mar 4, 2025

hey @kaf-lamed-beyt! Thanks for throwing this together!

When running this on ipfs-webui (build, npm pack, git cherry-pick d751fc63903bc72086b177f82bc4933136e89d0f, then install on ipfs-webui) I am still seeing some un-dark styled components:

image

some issues:

  1. breadcrumb in top left is barely visible
  2. white backgrounds on content in page
  3. text on right side is difficult to see

Also, please make sure you're using coloring from ipfs-css (source at https://github.com/ipfs-shipyard/ipfs-css/blob/main/ipfs.css#L282-L562)

@kaf-lamed-beyt
Copy link
Author

ah yes, i couldn't test the changes I made.

git cherry-pick d751fc63903bc72086b177f82bc4933136e89d0f

I'm guessing this is a commit hash? which I guess would also change. I have no idea what this does, @SgtPooki, if you don't mind elaborating, that'll be helpful.

Also, please make sure you're using coloring from ipfs-css (source at https://github.com/ipfs-shipyard/ipfs-css/blob/main/ipfs.css#L282-L562)

okay, I'll check this out too.

@SgtPooki
Copy link
Member

I'm guessing this is a commit hash? which I guess would also change. I have no idea what this does, @SgtPooki, if you don't mind elaborating, that'll be helpful.

Yes, it's a commit hash for the ipfs-webui hash where you added dark mode: ipfs/ipfs-webui@d751fc6

So, with two terminals:

  1. term1: have ipfs-webui repo opened, checkout/apply that commit hash
  2. term2: make changes in ipld-explorer-components, run npm pack
  3. term1: do npm install --save <path_to_ipld-explorer-components>.tgz
  4. term1: npm run start and test things out

you could also add some bootstrapping/basic code to toggle darkmode in ipld-explorer-components the same way we do in ipfs-webui. That's the ideal way to do it, but is more work.

Ideally we would have some shared component that adds darkmode toggle in the same way, that we could import in both these projects, but i don't want to block you on that.

@kaf-lamed-beyt
Copy link
Author

thank you so much for your response, @SgtPooki. I'll follow the multiple terminal steps and let you know how it goes.

you could also add some bootstrapping/basic code to toggle darkmode in ipld-explorer-components the same way we do in ipfs-webui. That's the ideal way to do it, but is more work.

do you mean something like adding a dark-theme provider/context kinda thing, the same way, we did previously in the initial ipfs-webui dark mode PR?

Ideally we would have some shared component that adds darkmode toggle in the same way, that we could import in both these projects, but i don't want to block you on that.

If it is related to adding a theme-provider in ipld-explorer-components, I don't mind giving it a shot. If not, can you shed more light how I could approach this on a high-level?

@SgtPooki
Copy link
Member

If it is related to adding a theme-provider in ipld-explorer-components, I don't mind giving it a shot. If not, can you shed more light how I could approach this on a high-level?

Honestly, i was thinking something more like adding a theme/context provider into either ipfs-css package, or https://github.com/ipshipyard/helia-react-components or something. but that is a big derailing that I don't think we need to undertake now :)

if you want to add a theme/context provider that is only enabled in dev/test mode in ipld-explorer-components for now i think that would be reasonable, but we won't want it in the final bundle

@kaf-lamed-beyt
Copy link
Author

if you want to add a theme/context provider that is only enabled in dev/test mode in ipld-explorer-components for now i think that would be reasonable, but we won't want it in the final bundle

alright, I'll try leaning towards this @SgtPooki. How does having an "examples" (where I test this out) folder sound? I'll look at how to exclude the dir from the builds depending on the bundler that ipld-explorer uses.

One more question, though... Since I've already updated my branch based on the feedback I got from @lidel regarding the tootips and modals. From my git log, I have this commit hash:

commit a911eb99d061417225785bb563c6d62a1de2d896 (HEAD -> dark-mode)
Author: kaf-lamed-beyt 
Date:   Fri Feb 28 00:27:32 2025 +0100

    refactor: improve more components appearance based on current theme

I just haven't pushed it. Should push it and open a PR so this 👇🏼 becomes possible?

term1: have ipfs-webui repo opened, checkout/apply that commit hash (the new commit hash above)

@SgtPooki
Copy link
Member

I just haven't pushed it. Should push it and open a PR so this 👇🏼 becomes possible?

You can just stay on that branch where you have the new ipfs-webui changes, and npm i --save <ipld-explorer-components-tarball>. You don't need to push the commit/hash/tag/ref to github to be able to build it.

@kaf-lamed-beyt
Copy link
Author

@SgtPooki, sorry for circling back to this late.

I followed the two terminal instructions you gave me, and I'm getting these errors after doing npm install --save ../ipld-explorer-components/ipld-explorer-components-8.1.3.tgz

ERROR in ./src/explore/ExploreContainer.js 6:0-61
Module not found: Error: Can't resolve 'ipld-explorer-components/pages' in '/oss/ipfs-webui/src/explore'
ERROR in ./src/explore/StartExploringContainer.js 6:0-68
Module not found: Error: Can't resolve 'ipld-explorer-components/pages' in '/oss/ipfs-webui/src/explore'
ERROR in ./src/explore/explore-page-renderer.jsx 9:0-74
Module not found: Error: Can't resolve 'ipld-explorer-components/providers' in '/oss/ipfs-webui/src/explore'
ERROR in ./src/explore/explore-page-renderer.jsx 10:0-38
Module not found: Error: Can't resolve 'ipld-explorer-components/css' in '/oss/ipfs-webui/src/explore'
ERROR in ./src/files/FilesPage.js 34:0-64
Module not found: Error: Can't resolve 'ipld-explorer-components/providers' in '/oss/ipfs-webui/src/files'
ERROR in ./src/index.js 16:0-84
Module not found: Error: Can't resolve 'ipld-explorer-components/providers' in '/oss/ipfs-webui/src'
ERROR in ./src/files/explore-form/files-explore-form.tsx 14:0-64
Module not found: Error: Can't resolve 'ipld-explorer-components/providers' in '/oss/ipfs-webui/src/files/explore-form'

@SgtPooki
Copy link
Member

@kaf-lamed-beyt you want to make sure to build ipld-explorer-components npm run build before you pack it.

@kaf-lamed-beyt
Copy link
Author

oh! my bad! i'll do that now, @SgtPooki! Thank you so much!

@kaf-lamed-beyt
Copy link
Author

@SgtPooki, I'm currently blocked;

The classes/vars i apply do not receive the appropriate dark theme styles. I tried locating the exact elements via dev tools, and whenever I try applying the classes to the element, nothing changed. I've just pushed now, perhaps when you take a look, you could help me identify where the issue is coming from.

The breadcrumb component especially! I also noticed that ObjectInfo uses react-inspector, modifying its style with the style attribute doesn't really change anything. Even when I do !important

 /* style for the react-inpector package that ObjectInfo uses */
  .r-inspector ol, li {
    background-color: var(--gray-muted) !important;
  }

The LargeLinksTable too.

So far... I've been able to update the text style in the CIDInfo component on the right. My only blockers are the breadcrumb path, objectInfo, and table styles.

image

@SgtPooki
Copy link
Member

@kaf-lamed-beyt ok let me take a look

@SgtPooki
Copy link
Member

SgtPooki commented Mar 18, 2025

@kaf-lamed-beyt check out kaf-lamed-beyt#1

it still needs some fine-tuning but i've got all the things colorized.. I also added a dev only dark theme toggle to help with development.

chore: colorize all components
@kaf-lamed-beyt
Copy link
Author

Ah! thank you so much for the PR @SgtPooki! I just merged it now. What else do you think I can add? Or is this okay for now?

@SgtPooki
Copy link
Member

Ah! thank you so much for the PR @SgtPooki! I just merged it now. What else do you think I can add? Or is this okay for now?

I think we want to normalize the colors. @lidel has a better eye for styling and accessibility than i do.

I think we should fine-tune the colors and then this should be good to go

@kaf-lamed-beyt
Copy link
Author

oh okay, perhaps when @lidel provides more info, I'll know what next to do. thank you, @SgtPooki!

@SgtPooki
Copy link
Member

oh okay, perhaps when @lidel provides more info, I'll know what next to do. thank you, @SgtPooki!

image
  1. colors and text of heading here need fixed
  2. node svg circles are not visible, needs fixed.
  3. header colors should transition to darker
  4. background color is not matching the different sections: needs fixed

@kaf-lamed-beyt
Copy link
Author

huh. this is strange, @SgtPooki. Here's what I can see on my end.

image

@SgtPooki
Copy link
Member

huh. this is strange, @SgtPooki. Here's what I can see on my end.

I was testing this in isolation, which wasn't getting any styling from ipfs-webui.. so it's good there. Can we make sure the styles look similar in ipld-explorer-components when not rendered within ipfs-webui?

@kaf-lamed-beyt
Copy link
Author

kaf-lamed-beyt commented Mar 24, 2025

So, I guess I'll need to test the examples (dev) folder, yeah?

@SgtPooki
Copy link
Member

So, I guess I'll need to test the examples (dev) folder, yeah?

I don't think so.. i'm just saying that the screenshot i provided was from npm run start in the ipld-explorer-components repo.

the screenshot you provided looks like you were rendering that from within the ipfs-webui repo?

@kaf-lamed-beyt
Copy link
Author

the screenshot you provided looks like you were rendering that from within the ipfs-webui repo?

Ah, yes. this is correct. I'll try testing with the npm run start command.

Is there a reason why it renders differently in the webui repo, and not the same way in ipld-explorer? @SgtPooki

@SgtPooki
Copy link
Member

Is there a reason why it renders differently in the webui repo, and not the same way in ipld-explorer? @SgtPooki

I'm guessing it's because of the CSS you added in ipfs-webui

@kaf-lamed-beyt
Copy link
Author

this is what it looks like on my end, @SgtPooki.

image

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.

2 participants