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.

Sign up for GitHub

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

Jump to bottom

🥔✨ Journal: Sprout Keywords#show #1649

Merged
merged 5 commits into from
Jul 20, 2023
Merged

🥔✨ Journal: Sprout Keywords#show #1649

merged 5 commits into from
Jul 20, 2023

Conversation

zspencer
Copy link
Member

@zspencer zspencer commented Jul 10, 2023 •
edited
Loading

@zspencer zspencer added ✨ feature Reduces Client's Burden or Grants them Benefits 🥔 Satisfices It's good enough to use, but not particularly great labels Jul 11, 2023
@zspencer zspencer force-pushed the journal/introduce-terms branch from 632529a to e3fd349 Compare July 13, 2023 00:11
@zspencer zspencer force-pushed the journal/expose-terms branch 2 times, most recently from 4fae22e to 994c958 Compare July 13, 2023 22:25
@zspencer zspencer force-pushed the journal/introduce-terms branch 2 times, most recently from 1f7e22e to 06d3df7 Compare July 13, 2023 22:34
@zspencer zspencer force-pushed the journal/expose-terms branch 3 times, most recently from 435b8b2 to 0f540b5 Compare July 13, 2023 22:38
Base automatically changed from journal/introduce-terms to main July 13, 2023 22:38
@zspencer zspencer changed the title 🥔✨ Journal: Sprout Terms#show 🥔✨ Journal: Sprout Keywords#show Jul 13, 2023
@zspencer zspencer force-pushed the journal/expose-terms branch from 0f540b5 to 11d2454 Compare July 13, 2023 22:45
@zspencer zspencer marked this pull request as ready for review July 13, 2023 22:47
@zspencer zspencer requested review from a team July 13, 2023 22:58
@zspencer zspencer marked this pull request as draft July 13, 2023 23:18
@zspencer
Copy link
Member Author

Actually, I think this needs moar love, and probably wants to come after #1663 so the keyword show page can show the Entry anyway!

@zspencer zspencer force-pushed the journal/expose-terms branch from 11d2454 to 54eb65c Compare July 13, 2023 23:33
@zspencer zspencer changed the base branch from main to journal/entries-know-their-keywords July 13, 2023 23:33
@zspencer zspencer force-pushed the journal/expose-terms branch from 54eb65c to 417844b Compare July 15, 2023 00:41
@zspencer zspencer force-pushed the journal/entries-know-their-keywords branch from 6f9a625 to 6ffd6bd Compare July 15, 2023 01:01
Base automatically changed from journal/entries-know-their-keywords to main July 15, 2023 01:07
@zspencer zspencer force-pushed the journal/expose-terms branch 2 times, most recently from 0560064 to 8bcca39 Compare July 15, 2023 01:47
@zspencer zspencer marked this pull request as ready for review July 15, 2023 02:05
@zspencer zspencer requested review from a team and removed request for a team July 15, 2023 02:05
anaulin
anaulin approved these changes Jul 20, 2023
View reviewed changes
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.

Codewise LGTM, but I feel like the UI needs a bit more context. Just looking at the screenshot I had no idea what I was looking at, or what the UI was intended for.

Is it meant to be something like:
"""

Other keywords related to 'tested'

  • tests
  • testing
    """

Or something else?

I'm not clear on what is the use case here.

Is this for a reader of a Journal to find keywords aliases? What for? If we wanted to show a reader all posts under a topic we could just show them a list of all entries with a keyword and any of its aliases.

Or is this UI for a journal owner to manage aliases? Or something else?

Regardless, nothing blocking for merge.

app/furniture/journal/keywords/show.html.erb Outdated
<h1><%= keyword.canonical_keyword %></h1>
<ul>
<%= keyword&.aliases&.each do |aliass| %>
<li><%= aliass %></li>
Copy link
Member

Choose a reason for hiding this comment

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

sssssssssss (thanks, Ruby)

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 was a/nnoyed/mused at the consequence of choosing aliases as a collection; and may wind up finding a different name to prevent the silliness.

@zspencer
Copy link
Member Author

Right now I am thinking it's just going to enumerate the entries that reference the keyword; but there are some other places I want to go:

  • Showing a Definition for a Keywords (or the Entry which serves as the canonical definition for the Keyword?)
  • Managing Aliases and Merging Keywords
  • Subscribe to Entries with the Keyword

I suppose I could start with {journal_path}/entries?keyword[]=test rather than /keywords/{keyword} for the Filtering and Subscribing toEntries by Keyword...

@zspencer zspencer force-pushed the journal/expose-terms branch from fce5551 to 39f59ab Compare July 20, 2023 17:29
@zspencer zspencer merged commit 2bb76ab into main Jul 20, 2023
@zspencer zspencer deleted the journal/expose-terms branch July 20, 2023 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anaulin anaulin anaulin approved these changes

Assignees
No one assigned
Labels
✨ feature Reduces Client's Burden or Grants them Benefits 🥔 Satisfices It's good enough to use, but not particularly great
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zspencer @anaulin