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

Journal: Initial MVP #900

Merged
merged 13 commits into from
Oct 30, 2022
Merged

Conversation

zspencer
Copy link
Member

@zspencer zspencer commented Oct 23, 2022

See: Furniture: Journal

I wanted to explore what it would look like to do a pretty standard Rails-only No TurboLinks piece of Furniture; and this is what I got!

journal-demo.mp4

@KellyAH
Copy link
Contributor

KellyAH commented Oct 23, 2022

If this "standard Rails-only No TurboLinks" way of implementing furniture is faster/easier to build/maintain/debug, that's great. But what risks do we take or features do we lose by not using TurboLinks?

The UI/UX for this feature still could use some improvements in a future iteration. Will that be possible with this current implementation?

@zspencer
Copy link
Member Author

re: no Hotwire - Theoretically, the way Rails apps are supposed to work is you:

  1. Build it without using Hotwire/Turbo, so that everything works without JavaScript
  2. Layer in turbo frames/streams as appropriate
  3. Profit!

So it should be relatively straightforward to layer in hotwire to improve the UX.

@zspencer zspencer force-pushed the journal/author-may-publish-journal-entries branch from af8eb4f to 3880216 Compare October 24, 2022 00:10
Copy link
Member

@anaulin anaulin left a comment

Choose a reason for hiding this comment

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

I have a couple of comments below, the one about Journal::Room more important to me than the other one. However, neither of these two is blocking for merging this -- feel free to ignore or not the comments and then merge when you're ready.

link 'Edit', [:edit, entry.space, entry.room, entry]
end
rescue NoMethodError => e
throw e unless e.message.start_with?("undefined method `crumb'")
Copy link
Member

Choose a reason for hiding this comment

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

What's this about? In which cases would we be throwing an undefined method crumb error, and why is that ok to swallow?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK this is HORRIBLE and I HATE IT but apparently Gretel's way of re-loading breadcrumbs uses an instance_eval; but if gets autoloaded by Rails and then it explodes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed it by adding breadcrumbs to Zeitwerks ignore directive!

@@ -0,0 +1,7 @@
class Journal::Room < Room
has_many :journal_entries, inverse_of: :room, class_name: 'Journal::Entry'
Copy link
Member

Choose a reason for hiding this comment

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

Is the only purposes of this new Room class to have the :journal_entries association?

I don't love it. It seems confusing to me, overloaded with our Room-Room, and causing us to have bits of code like room.becomes(Room)....

If the main purpose of this is to load the entries in Journal (without adding a has_many to our proper Room), I'd prefer we removed this class and instead implemented entries with an ActiveRecord lookup, like Entry.where(room_id: room).recent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya, this gets wiped in the fancier version of the PR.

@zspencer zspencer marked this pull request as draft October 27, 2022 00:14
@zspencer
Copy link
Member Author

Moved to draft because I wanna do it like we did the Marketplace.

zspencer and others added 8 commits October 30, 2022 10:02
- [ ] Authorize + Policy Scope!
- [ ] Styling!
- [ ] Render the Entry HTML using Markdown
OK, this does *way* too much in one commit but YOLO. This:

1. Switches our Markdown Renderer from RedCarpet to Commonmarker
   because that's what Github uses :X
2. Updates `gretel` to support defining Breadcrumbs in Furniture!
3. Sprouts a `datetime_field` partial for rendering `datetime` HTML
   fields
4. Creates a Link between Person and Rooms through Spaces for
   permissioning ease.
5. Gives Journal Entries a Slug
6.
Co-authored-by: KellyAH <[email protected]>
Well, I couldn't figure out how to get the heading anchors injected
using CommonMarker, and I really think that's important so :X
Apparently Gretel uses `instance_eval` to load it's breadcrumbs up;
which is why `crumb` is available on `Object`.

This is kinda a hacky-way to deal with that; but it may make sense for
us to exclude the breadcrumb paths from autoloading somehow?
@zspencer zspencer force-pushed the journal/author-may-publish-journal-entries branch from 3880216 to 5c74d19 Compare October 30, 2022 17:03
OK some stuff to think through:

1. Inheriting from FurniturePlacement could let us get rid of
   `Placeable`, which would be nice!
2. We could also then rename from `FurniturePlacements` to `Furniture`
@zspencer zspencer marked this pull request as ready for review October 30, 2022 18:15
@zspencer zspencer merged commit e67c767 into main Oct 30, 2022
@zspencer zspencer deleted the journal/author-may-publish-journal-entries branch October 30, 2022 19:02
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