Add page from Builder #2192
Add page from Builder #2192
Conversation
Available types:
|
|
The responsiveness certainly leaves something to be desired
|
| /> | ||
|
|
||
| <InputText | ||
| align="left" |
There was a problem hiding this comment.
This and the value for size are invalid. I have it set up so it should be mapping to correct values, I need to talk to their team about that.
There was a problem hiding this comment.
Okay I figured out what was going on with this one and it is now correctly mapping
| <InputFile | ||
| allowedTypes="all" | ||
| variation="dropzone" | ||
| className={styles.fileInput} |
There was a problem hiding this comment.
It is still adding className and onChange even though I am explicitly telling it not to
| }) => ( | ||
| <div className={styles.securityBlurb}> | ||
| <Icon name="lock" size="small" className={styles.securityIcon} /> | ||
| <div className={styles.securityText}> |
There was a problem hiding this comment.
huh, styles.securityText just straight up doesn't exist 😬
| variation="dropzone" | ||
| className={styles.fileInput} | ||
| allowMultiple | ||
| onChange={setFiles} |
There was a problem hiding this comment.
Input file requires a getUploadParams prop. There is no equivalent in Figma but I added it to the mapping file so I would expect it to be added here. I am waiting to hear back from their team on that
There was a problem hiding this comment.
there are a handful of styles that I'm pretty skeptical of. they either do nothing, or are a questionable way of achieving the desired result.
one example is the .container class having equal padding AND justify-content/align-items center. those are both doing the same thing, trying to center the content except only the padding is doing what it is supposed to do since the container has no provided dimensions to center its content within.
at first glance this seems reasonable making it easy to miss and leave behind some code that is not needed. also, we can't ask the "author" what they were going for.
There was a problem hiding this comment.
it's also oddly inconsistent with what it solves a problem with
in the .container it used display: flex to fill the width, which is valid.
then in a handful of other classes it used width:100% to do the same thing, which is once again superfluous since it's applying that to a div which has display: block. the rule isn't providing the wrong behavior it's just that you don't need to even apply that rule for it to do the right thing.
There was a problem hiding this comment.
Great callout! I also have some concerns about unnecessary CSS. It currently has no awareness of our components so I have seen it wrapping components with a size prop in a div specifying what the size should be. Potentially fixable by telling the AI "if you encounter a component with a size prop don't use CSS to control its size". This is brittle for two reasons:
- If a component comes along where a prop other than "size" controls its size
- More likely a component has a default size value so there is no size prop provided.
InputTextfor example takes a size prop of"small" | "large"and if none are provided it uses the value of "base" (arguably bad design not allowing the default value as a prop but that is the current situation). If no size prop is provided it will assume it needs to wrap it in a div and add its own CSS to size it.
This isn't an issue right now because it isn't noticeable but I see this becoming a problem if we want to change the size in the future. The developer will change the size prop and be confused about why it isn't working. Turns out there is also CSS now dictating the wrong size built in
| <div className={styles.securityBlurb}> | ||
| <Icon name="lock" size="small" className={styles.securityIcon} /> | ||
| <div className={styles.securityText}> | ||
| Documents and files you upload are stored securely as per our{" "} |
There was a problem hiding this comment.
a couple things here
- first it's putting text directly in a div rather than some kind of text element which I don't love
- second, the
{" "}is an interesting way to add a space. I get that a formatter might remove a trailing whitespace but still. I don't love it.
This draft PR includes a page generated using builder.io based on this design.
Custom Prompts Used
Prompts from builder.io that are editable
Follow these fundamental principles when generating code:
Code Quality:
• Write clean, maintainable, and concise code
• Follow DRY (Don't Repeat Yourself) principles
• Ensure proper error handling
Documentation:
• Add meaningful comments for complex logic
• Document any assumptions or limitations
Naming Conventions:
• Use descriptive and consistent naming
• Follow language-specific conventions
• Avoid abbreviations unless widely recognized
Implement WCAG-compliant, accessible code following these essential requirements:
Semantic Structure:
• Use semantic HTML elements (nav, main, article, section)
• Create logical heading hierarchy (h1-h6)
• Implement proper landmark regions
Interactive Elements:
• Ensure keyboard navigation support
• Maintain visible focus indicators
• Implement proper focus management
• Support touch and mouse interactions
ARIA Implementation:
• Add appropriate ARIA landmarks
• Include descriptive ARIA labels
• Use ARIA states and properties correctly
• Implement ARIA live regions for dynamic content
Media and Content:
• Provide descriptive alt text for images
• Ensure proper color contrast ratios
• Support text resizing and zoom
• Include captions and transcripts for media
Testing Requirements:
• Verify screen reader compatibility
• Test with keyboard-only navigation
• Ensure functionality at 200% zoom
Testing
You can take a look at the page here
Styles mappings
Currently waiting to hear back from their team about doing the styling mappings as there is a gap in their docs about it
In Atlantis we use Github's built in pull request reviews.