ToposInstitute / CatColab

A collaborative environment for formal, interoperable, conceptual modeling
https://catcolab.org
MIT License
11 stars 4 forks source link

Styled tooltips for buttons #131

Closed epatters closed 2 days ago

epatters commented 2 weeks ago

We now have an IconButton component used in several places. These should have a prop to specify a tooltip that will appear on hover.

It shouldn't be necessary to roll this ourselves. We could use Corvu's tooltip component, for example.

AlexVCS commented 2 weeks ago

Hi there, I'd like to work on this issue. Can you assign it to me please?

AlexVCS commented 2 weeks ago

I cloned the project, ran npm install no problem, but am running into errors when I run npm run build. This error is in five different files where RandomState is used:

error[E0432]: unresolved import `std::hash::RandomState`
  --> packages/catlog/src/dbl/theory.rs:70:56
   |
70 | use std::hash::{BuildHasher, BuildHasherDefault, Hash, RandomState};
   |                                                        ^^^^^^^^^^^ no `RandomState` in `hash`
   |
   = help: consider importing this struct instead:
           std::collections::hash_map::RandomState

Do you have any recommendations on how to remedy this?

epatters commented 2 weeks ago

Hi @AlexVCS, thanks for offering to work on this issue!

The problem seems to be that you need a more recent version of Rust. I've updated the Cargo.toml to reflect this (#132). I'd suggest updating to the latest stable version of Rust. The tool rustup is very convenient for this if you're not already using it.

We should probably also update the developer instructions in the README with info about getting set up with Rust.

AlexVCS commented 2 weeks ago

Hi @AlexVCS, thanks for offering to work on this issue!

The problem seems to be that you need a more recent version of Rust. I've updated the Cargo.toml to reflect this (#132). I'd suggest updating to the latest stable version of Rust. The tool rustup is very convenient for this if you're not already using it.

We should probably also update the developer instructions in the README with info about getting set up with Rust.

Thanks for letting me work on it!

You were right about that thanks! I updated Rust to 1.80.1 and it got me through the build step.

Now when I run npm run dev and navigate to http://localhost:5173/ I just get a white screen!

Screenshot 2024-08-29 at 11 02 04 AM

Any ideas on why this is happening and how to fix it?

epatters commented 2 weeks ago

Ah, sorry, I think you've hit another bump we need to smooth out. Try running

export VITE_BACKEND_HOST=backend-next.catcolab.org

in bash before running npm run dev.

AlexVCS commented 2 weeks ago

No prob. I've tried running that in bash and then ran npm run dev but the page still looks the same. Is there anything else I need to run?

epatters commented 2 weeks ago

Not that I know of. Do you see any errors in the browser console? Also, as a sanity check, does the app come up when you go to https://next.catcolab.org?

AlexVCS commented 2 weeks ago

Not that I know of. Do you see any errors in the browser console? Also, as a sanity check, does the app come up when you go to https://next.catcolab.org?

I'm getting this error despite running the script in bash:

chunk-ISFT24YX.js?v=d776f89f:1039 Uncaught Error: Must set environment variable BACKEND_HOST
    at castError (chunk-ISFT24YX.js?v=d776f89f:1039:10)
    at handleError (chunk-ISFT24YX.js?v=d776f89f:1052:17)
    at runComputation (chunk-ISFT24YX.js?v=d776f89f:773:12)
    at updateComputation (chunk-ISFT24YX.js?v=d776f89f:738:3)
    at createMemo (chunk-ISFT24YX.js?v=d776f89f:264:10)
    at [solid-refresh]App (@solid-refresh:22:20)
    at chunk-ISFT24YX.js?v=d776f89f:597:14
    at untrack (chunk-ISFT24YX.js?v=d776f89f:482:12)
    at Object.fn (chunk-ISFT24YX.js?v=d776f89f:593:11)
    at runComputation (chunk-ISFT24YX.js?v=d776f89f:759:22)Caused by: Must set environment variable BACKEND_HOST

The site took a little while to load but did come up!

epatters commented 2 weeks ago

Ah, so it is related to the environment variable. I get the same error when the environment variable isn't set, but the following works for me when run in bash:

$ VITE_BACKEND_HOST=backend-next.catcolab.org npm run dev
epatters commented 2 weeks ago

With #136, you should no longer need to set any environment variables before running npm run dev. Hope that's the last of the setup issues!

AlexVCS commented 2 weeks ago

With #136, you should no longer need to set any environment variables before running npm run dev. Hope that's the last of the setup issues!

You're right! I have it running locally now! Is the idea to wrap the parent IconButton component in the corvu tooltip, or to wrap each instance of the IconButton in the tooltip component? I started with changing the parent IconButton (early idea of that below), but wanted to check.

import {type JSX, splitProps} from "solid-js";
import Tooltip from "@corvu/tooltip";

import "./icon_button.css";

/** Styled, unobstrusive button intended to include an icon.
 */
export function IconButton(
  allProps: {
    children: JSX.Element;
  } & JSX.ButtonHTMLAttributes<HTMLButtonElement>
) {
  const [props, buttonProps] = splitProps(allProps, ["children"]);

  return (
    <Tooltip>
      <Tooltip.Trigger />
      <Tooltip.Portal>
        <Tooltip.Content>
          <button class="icon-button" {...buttonProps}>
            {props.children}
          </button>
          <Tooltip.Arrow />
        </Tooltip.Content>
      </Tooltip.Portal>
    </Tooltip>
  );
}
epatters commented 2 weeks ago

Great! That's exactly what I had in mind. Most of these buttons will have tooltips, so it makes sense to put the tooltip component inside it to reduce boilerplate.