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

Fix open with for note kind #1 #497

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

ticruz38
Copy link
Collaborator

@ticruz38 ticruz38 commented Nov 29, 2024

#455

The rendering architecture needs to change, since we switch on kind only within NoteContent. With e.g. feeds, it will make more sense to swap out rendering for the entirety of the note instead. Kind handlers should only show up on non-kind-1 notes, and probably be integrated more tightly with the content. Also use a different icon.

I don't have the context or state of Coracle at the time of this comment but from what I can see in the code, I have some questions:

  • You do still switch NoteContent based on kind.
  • By entirety of the note do you mean having NoteActions embedded in NoteContent? I like both being separated still.
  • Kind Handlers are tightly integrated with the note content, as it seems each handlers has a kind "attached" to it. You are already switching handlers based on the Note kind. Or you want to use another means, aside from the note kind, to filter out the Note handler?
  • The icon choice is quite limited on Font-Awesome, I came up with the fa-list icon, which seems more appropriate, for a wider range of icon I personally use iconify, but that's a big departure from font-awesome, even though iconify support font-awesome too. It does add some dependencies that you need to be confortable with.

@ticruz38
Copy link
Collaborator Author

ticruz38 commented Dec 2, 2024

https://github.com/ticruz38/coracle/tree/fix/open-with-iconify

Check this branch out for an iconify version

@staab staab changed the base branch from master to dev December 2, 2024 17:29
@staab
Copy link
Collaborator

staab commented Dec 2, 2024

The iconify version looks better, but we shouldn't add a dependency. See if you can get something like that into Icon.svelte in svg format (although with an arrow would be preferable). I often use https://www.flaticon.com/ for stuff like this.

For an illustration of what I mean, certain note kinds should ideally be able to take over the layout of the profile picture etc. to differentiate them from normal notes. Here's how a feed event currently looks:

Screenshot 2024-12-02 at 9 40 55 AM

Which obviously is not ideal. Here's an example of how it might look:

Screenshot 2024-12-02 at 9 46 25 AM

This is going to be very hairy to accomplish, since it'll require allowing NoteContentX components to call NoteActions internally, or maybe use crazy slots. Then, of course, there are quotes, which are already convoluted to avoid circular dependencies. The simplest solution might be to add logic to Note.svelte, but that's going to be ugly too. I'd really like the solution to be very clean, since there is so much mission-critical stuff going on in these files.

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