Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Works but violates progressive disclosure
Taste Rating: The skill solves a real problem but over-engineers the solution with massive reference files that defeat the purpose of progressive disclosure.
Verdict: ✅ Worth merging with improvements - Core structure is sound, but reference files need refactoring
Key Insight: Progressive disclosure means the agent loads only what it needs when it needs it. A 527-line style presets file defeats this purpose - split it into individual preset files so agents load only the style they choose.
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - But with a DRY violation
Taste Rating: The progressive disclosure structure is good, but one preset file has duplicated global content that breaks consistency.
Verdict: ✅ Worth merging after fixing the duplication in paper-ink.md
Key Insight: When you split content into separate files for progressive disclosure, make sure EVERY file follows the same pattern. One outlier defeats the purpose.
|
@OpenHands Use I think if we keep very close to the original, but DRY and reference, respectively split in workflow files, we’re still good. Fix them one by one and reply in each review thread with your fix and explanations, be reassuring that it’s solved and good, and mark them as solved. |
|
I'm on it! enyst can track my progress at all-hands.dev |
Co-authored-by: openhands <openhands@all-hands.dev>
|
No additional code or docs changes were made since my last update. Completion checklist (per your request)
Notes on instructions
|
|
@OpenHands Use I think if we keep very close to the original, but DRY and reference, respectively split in workflow files, we’re still good. Fix them one by one and reply in each review thread with your fix and explanations, be reassuring that it’s solved and good, and mark them as solved. |
|
I'm on it! enyst can track my progress at all-hands.dev |
Co-authored-by: openhands <openhands@all-hands.dev>
Final summary (work completed)Since my last update, there were no additional code or documentation changes beyond the already-pushed fix. What I did (and status)
Conciseness check
|
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable
Taste Rating: The progressive disclosure structure is solid, and the skill solves a real problem with proven success (1.5k+ stars). Previous reviews caught and fixed the major architectural issues.
Verdict: ✅ Worth merging after fixing the magic number in example code.
Key Insight: Battle-tested code that works in production should not be over-engineered. The remaining complexity (mandatory viewport CSS, etc.) is solving real problems, not imaginary ones.
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable
Taste Rating: Progressive disclosure is solid, skill solves a real problem (1.5k+ stars), and previous review rounds caught most issues.
Verdict: ✅ Worth merging after fixing the incomplete code example.
Key Insight: When teaching agents by example, incomplete code snippets create copy-paste traps. Show the full pattern.
|
@OpenHands Investigate this PR and understand its goal and implementation very well. Address the remaining issues with this PR to prepare it for merge; including CI / conflicts / reviews. Use gh to see all comments and inline reviews to address. You are allowed to respond to the reviews as you see fit, use your best judgment. Make it ready. Then ask for the same review bot - the same! - for a review. |
|
I'm on it! enyst can track my progress at all-hands.dev |
- merge latest main to resolve PR conflicts and pick up current marketplace layout - add a usage example for TiltEffect in ANIMATION_PATTERNS.md - update AGENTS.md marketplace registration guidance to match the current repo structure Co-authored-by: openhands <openhands@all-hands.dev>
|
Final pass complete for this PR:
Requested the same review bot again: @all-hands-bot |
This comment was marked as duplicate.
This comment was marked as duplicate.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable
Taste Rating: Progressive disclosure is solid, code examples are complete, and previous review rounds fixed the major issues.
Verdict: ✅ Worth merging after removing the orphaned heading.
Key Insight: When you fix an issue once (orphaned headings in dark-botanical.md), grep for the same pattern everywhere. Consistency matters.
- remove the trailing '## Specialty Themes' heading from the vintage-editorial preset - verify no other preset file ends with the same orphaned heading pattern Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed the latest review too:
Re-requested the same review bot: @all-hands-bot |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Progressive disclosure is solid and the skill has proven success. One DRY violation needs cleanup.
| | Slide Type | Maximum Content | | ||
| |------------|-----------------| | ||
| | Title slide | 1 heading + 1 subtitle | | ||
| | Content slide | 1 heading + 4-6 bullets (max 2 lines each) | | ||
| | Feature grid | 1 heading + 6 cards (2x3 or 3x2) | | ||
| | Code slide | 1 heading + 8-10 lines of code | | ||
| | Quote slide | 1 quote (max 3 lines) + attribution | | ||
|
|
||
| Too much content? Split into multiple slides. Never scroll. |
There was a problem hiding this comment.
🟠 Important: Content density limits table duplicated with inconsistent data.
The Problem:
This exact table exists in VIEWPORT_FITTING.md (lines 15-25) but with different values:
| Difference | This File | VIEWPORT_FITTING.md |
|---|---|---|
| Title slide | "1 heading + 1 subtitle" | "1 heading + 1 subtitle + optional tagline" |
| Content slide | "4-6 bullets (max 2 lines each)" | "4-6 bullet points OR 1 heading + 2 paragraphs" |
| Image slide | ❌ Missing | ✅ "1 heading + 1 image (max 60vh height)" |
Why This Matters:
Two sources of truth with different data WILL drift over time. An agent reading both files sees conflicting guidance. Previous reviews caught and fixed the CSS duplication—this has the same root cause.
Fix:
Keep the canonical table in VIEWPORT_FITTING.md only. Replace this section with a reference:
## CRITICAL: Viewport Fitting (Non-Negotiable)
Every slide MUST fit exactly in the viewport. No scrolling within slides, ever.
**See [VIEWPORT_FITTING.md](VIEWPORT_FITTING.md) for:**
- Mandatory base CSS and responsive breakpoints
- Content density limits per slide type
- Overflow prevention checklist2 participants
Imports the
frontend-slidesAgentSkills skill from https://github.com/zarazhangrui/frontend-slides.Changes:
skills/frontend-slides/with condensedSKILL.mdand supporting reference docs inreferences/(to keep SKILL.md small per AgentSkills progressive disclosure guidance)..plugin/marketplace.json.@enyst can click here to continue refining the PR