-
Notifications
You must be signed in to change notification settings - Fork 41
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
Homepage Section #260
Homepage Section #260
Conversation
… the info section
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.
Hi Fatuma,
Great work. The code is basically fine, but with apologies, it's a shared codebase, so we need to be tidy about things like unused files and console warnings.
Essential fixes:
- Fix NotFound.jsx, still has a reference to RubyOutline that breaks
- Fix console warning (text below)
- Remove unused (image) files
- Remove the stock photo full of men :)
react-dom.development.js:86 Warning: Encountered two children with the same key, link-1
. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version.
I can't see any references to link-1, so my guess is it's a generated link when there isn't one, so the fix is to add an explicit name or label - I don't have more details, it's been a while since I last used react, happy to pair with you if you get stuck on this one
Optional fixes:
- All the other comments
- Add missing newlines at the ends of files
spec/system/visit_site_pages_spec.rb
Outdated
expect(page).to have_text('404') | ||
end | ||
end | ||
# it 'visits home page' do |
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.
I think most of these tests should still pass, I would reinstate them if it's quick, and just leave out any complex fixes until next year
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.
Agreed with Fatuma that we'll pick this up in the next PR, because she ran out of hours in December
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.
And then, once I uncommented, they were all passing anyhow!
…eak for the new redesign banner
Key Changes
Updated Layout:
New Sections Added:
Styling and Branding:
Improved Responsiveness:
Ensured that all components adapt seamlessly across devices.