-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
🎉 UI updates for standardized explorer names, explore tagging, and an explorer-tiles
component
#3158
Conversation
WalkthroughWalkthroughThe update introduces a comprehensive feature for managing explorer tags within an administrative interface. It includes the creation of a new page for tag management, enhancements to existing components for navigation and display, and backend adjustments for data handling. The changes streamline the process of linking explorers to tags, improve the user interface for managing these associations, and refine data fetching and processing mechanisms across the system. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
10026dc
to
37e7c98
Compare
explorer-tiles
component
@coderabbitai review |
@@ -554,22 +566,28 @@ export class GdocBase extends BaseEntity implements OwidGdocBaseInterface { | |||
title: resolvedTitle, | |||
resolvedUrl: `${BAKED_GRAPHER_URL}/${resolvedSlug}`, | |||
thumbnail: `${BAKED_GRAPHER_EXPORTS_BASE_URL}/${resolvedSlug}.svg`, | |||
tags: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not doing anything with chart tags atm. Can add them when we need them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ikesau I'm seeing a failing test (can repro locally)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is so nice not to have to thread explorers throughout the codebase, well done! 🙌
Run locally, works as expected 🤝
I don't know how often this'll happen but it might be visually better to align the titles here
Mardown making its way into prominent links (I couldn't go all the way with running mirror_explorers
locally so I'm not sure if that would fix it so I'm raising it here)
<ul> | ||
<li> | ||
<Link to="/explorer-tags"> | ||
<FontAwesomeIcon icon={faTag} /> Explorer Tags | ||
</Link> | ||
</li> | ||
</ul> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably nothing to change now
this is a bit unexpected - but maybe unavoidable given the git/db limbo. When first encountering this, it gave me the impression that "explorer tags" were different from the regular ones.
Also given how seldom this is going to be used beyond the first tagging effort, it seems like there is a mismatch with the prime real estate this menu item will be permanently using.
Maybe worth considering complementing this with a more contextual link to /explorer-tags on every explorer row?
return res.send({ | ||
explorers: await db.getExplorerTags(db.knexInstance()), | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not absolutely necessary now, but maybe wrap in read-only transaction?
> | ||
{icon} | ||
<div className="explorer-tile__text-container"> | ||
<p className="h3-bold explorer-tile__title"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add background blur to this
The UI complement to owid/owid-content#36
Charts page:
Explorer Tiles component:
Explorer page:
Explorer prominent link:
Summary by CodeRabbit
New Features
Enhancements
explorerSubtitle
.Refactor
Bug Fixes
ProminentLink
component to handle various scenarios more effectively.Database Changes
explorer_tags
table to manage explorer-tag associations.