Skip to content

Change "Note" node styling and make it more versatile #2465

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

Merged
merged 29 commits into from
Jan 27, 2024

Conversation

joeyballentine
Copy link
Member

image

Unlike when I shared this before on Discord, this now just utilizes a new node type, but the schema is still defined on the backend. I still want that to change one day (specifically for this node only, but also for future things such as node groups that will also be implemented as "nodes that are only for visual purposes"). However, chaiNNer's plumbing just simply doesn't allow for this at the moment (at least not without getting a bunch of console errors).

@RunDevelopment
Copy link
Member

Sorry for the delay @joeyballentine!

node_type="note" is not a good idea. The main issue is that lineage rests upon the node type. Basically everything that handles lineage, needs the node type to figure out what it needs to do, and so this change will require a change to a lot of lineage code.

As I previously suggested, a simple solution would be to make Note a regular node, and let the frontend know about it. The backend node and frontend NoteNode component are already tightly coupled, so you might as well do a if (schemaId == "...:note") inside the component for regular nodes.

If you really detest checking schema IDs, you can also add something like a node_style: "standard" | "note" property to node data and control the design via that. This doesn't make the coupling between the backend and frontend for Note any less, but it's a system that we could take advantage of for other specialized note designs in the future.

@joeyballentine
Copy link
Member Author

Ok, so this now just makes an exception for this node by checking the schema id on the frontend. Some day I'd like to change this, but the way our state is, it just isn't really possible right now.

@RunDevelopment
Copy link
Member

A few things:

  1. image
    This is likely connected to the fact that MD mode shows weirdly padding at the bottom that results in this:
    image
  2. Left-clicking on markdown text does nothing. This is weird.
  3. The scroll bar in both text and MD mode is not usable with the mouse, because it largely overlaps with the hitbox for changing the width of the node.

Also, what do you think about using a WYSIWYG editor like bangle?

@joeyballentine
Copy link
Member Author

Left-clicking on markdown text does nothing. This is weird.

I would like to make this not selectable at least in this PR. I tried very hard to make every element selectable in it, but doing so then leaked selectability and made other things outside of it selectable. I just don't want to deal with that headache right now

The scroll bar in both text and MD mode is not usable with the mouse, because it largely overlaps with the hitbox for changing the width of the node

I thought of it as more of a visual indicator rather than a usable bar. What would be your recommended course of action for this?

Bangle

Yeah that looks sick, let's do it. Can I do that in a separate PR though?

@RunDevelopment
Copy link
Member

I thought of it as more of a visual indicator rather than a usable bar. What would be your recommended course of action for this?

Then let's leave it for now.

Yeah that looks sick, let's do it. Can I do that in a separate PR though?

Sure.

@joeyballentine
Copy link
Member Author

Ok, I've:

  1. Fixed the corner issue
  2. Made the scroll wheel interactive at the expense of making the sides not resizable anymore (so only the corner is). I don't think this matters too much
  3. also switched the overflow to overflowY so that it wouldn't add the padding at the bottom of the markdown where the x scrollbar would go

I think this is good to go now

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

Nice. Thank you!

@joeyballentine joeyballentine merged commit b5d0e1d into main Jan 27, 2024
@joeyballentine joeyballentine deleted the static-notes-node branch January 27, 2024 17:28
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