Banner overflow #49
Banner overflow #49
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new DemoBanner component, refines various layouts to accommodate the banner and logos, and cleans up unused code across the project.
- Added a reusable
DemoBannercomponent and removed the inline banner in_document.js - Updated
Layout,AppBar,BurgerHeader, andContentWrapperto adjust for banner space and injected logos - Cleaned up unused imports/props and tweaked UI components for consistent spacing
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| movies-app/parts/SummarySectionHeading/index.js | Updated top/bottom margins on summary headings |
| movies-app/parts/Layout/index.js | Integrated DemoBanner, added header logo |
| movies-app/parts/Layout/ContentWrapper/index.js | Increased top padding for content area |
| movies-app/pages/_document.js | Removed hardcoded inline banner HTML/CSS |
| movies-app/containers/Sidebar/index.js | Wrapped sidebar; added menu-container padding |
| movies-app/containers/Sidebar/SidebarInnerWrapper/index.js | Converted margin-top to padding-top |
| movies-app/containers/SearchBar/MagnifierButton/index.js | Adjusted search icon width to 1em |
| movies-app/containers/SearchBar/Form/index.js | Tweaked form padding/height for consistency |
| movies-app/containers/AppHeader/BurgerHeader/index.js | Added mobile header logo and accompanying styles |
| movies-app/components/UI/AppBar/index.js | Updated positioning and toolbar heights |
| movies-app/components/Menu/index.js | Removed unused Logo import and isMobile |
| movies-app/components/Menu/SectionHeading/index.js | Added top padding and position: relative |
| movies-app/components/DemoBanner/index.js | Created new DemoBanner component |
Comments suppressed due to low confidence (2)
movies-app/components/DemoBanner/index.js:3
- [nitpick] No tests were added for the new
DemoBannercomponent. Consider adding unit or snapshot tests to verify its appearance and behavior.
const DemoBanner = () => (
movies-app/parts/Layout/index.js:113
- The
DemoBannercomponent is rendered twice in the layout (lines 34 and 113). Remove the duplicate instance to avoid showing two banners.
<DemoBanner />
| padding-top: 100px; | ||
| padding-bottom: 6rem; | ||
| } | ||
| } | ||
|
|
||
| @media ${theme.mediaQueries.large} { | ||
| .content-wrapper { | ||
| padding-top: 72px; | ||
| padding-top: 100px; |
There was a problem hiding this comment.
[nitpick] This padding-top value is duplicated in two media queries. Extract the value into a constant or shared style to reduce duplication and ease future adjustments.
Copilot uses AI. Check for mistakes.
| <div className='logo-container'> | ||
| <img | ||
| className='logo-img' | ||
| width='56' | ||
| height='56' | ||
| src={LOGO_IMAGE_PATH} | ||
| alt='movie ticket' /> | ||
| </div> |
There was a problem hiding this comment.
[nitpick] The logo-container and logo-img styles are duplicated in both Layout and BurgerHeader. Consider abstracting this logo block into a shared component or CSS module to avoid repetition.
| <div className='logo-container'> | |
| <img | |
| className='logo-img' | |
| width='56' | |
| height='56' | |
| src={LOGO_IMAGE_PATH} | |
| alt='movie ticket' /> | |
| </div> | |
| <LogoBlock /> |
Copilot uses AI. Check for mistakes.
| <HamburgerButton onClick={openMenu} /> | ||
| <div className='left-section'> | ||
| <HamburgerButton onClick={openMenu} /> | ||
| <div className='logo-container'> |
There was a problem hiding this comment.
[nitpick] Same logo-container/logo-img duplication here. Extract into a reusable component or shared styles to keep the code DRY.
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
This pull request introduces a variety of changes to improve the UI/UX and code organization of the
movies-appproject. Key updates include adding a demo banner for testing purposes, refining the layout and styles of several components, and removing unused code. Below is a summary of the most important changes grouped by theme:New Features
DemoBannercomponent to display a fixed banner at the top of the application for demo purposes. This replaces the inline banner previously defined in_document.js. (movies-app/components/DemoBanner/index.js,movies-app/pages/_document.js,movies-app/parts/Layout/index.js) [1] [2] [3]Layout and Styling Improvements
AppBarcomponent to account for the new demo banner by modifying its position and reducing the toolbar height across different screen sizes. (movies-app/components/UI/AppBar/index.js) [1] [2] [3]ContentWrapperpadding to accommodate the demo banner and improve spacing consistency. (movies-app/parts/Layout/ContentWrapper/index.js)BurgerHeaderandLayoutcomponents by adding a logo to the header and adjusting their layouts for better alignment and spacing. (movies-app/containers/AppHeader/BurgerHeader/index.js,movies-app/parts/Layout/index.js) [1] [2] [3]Code Cleanup and Refactoring
Logoimport andisMobileprop from theMenucomponent to simplify the code. (movies-app/components/Menu/index.js) [1] [2] [3]Layoutto clean up the file. (movies-app/parts/Layout/index.js)Minor UI Adjustments
Formin theSearchBarcomponent to improve its appearance. (movies-app/containers/SearchBar/Form/index.js)MagnifierButton'sSearchIconfor better alignment. (movies-app/containers/SearchBar/MagnifierButton/index.js)SectionHeadingandSummarySectionHeadingcomponents. (movies-app/components/Menu/SectionHeading/index.js,movies-app/parts/SummarySectionHeading/index.js) [1] [2]Sidebar Enhancements
Sidebarin a new container to improve layout flexibility and added padding to the menu container for better spacing. (movies-app/containers/Sidebar/index.js,movies-app/containers/Sidebar/SidebarInnerWrapper/index.js) [1] [2]