Skip to content
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

Navbar #7

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Navbar #7

wants to merge 5 commits into from

Conversation

DevTrav
Copy link
Contributor

@DevTrav DevTrav commented Nov 10, 2024

Changes:

  • Updates to nav bar
  • Implementation of nav bar
  • Added scaffold of hero component
  • Implemented hero component place holder

@DevTrav DevTrav requested a review from a team as a code owner November 10, 2024 21:35
src/app/components/hero/index.tsx Outdated Show resolved Hide resolved
src/app/components/hero/index.tsx Show resolved Hide resolved
<Container
sx={{
position: 'relative',
zIndex: 2,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 can we accomplish this without z-index? Usually I try to avoid modifying z-index because it can lead to really challenging visual bugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Hero component example, the zIndex is used to control the layering of the overlay, background image, and text. It appears to be semantically aligned with MUI's best practices.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to just rely on the default stacking order for these elements.

src/app/components/hero/index.tsx Show resolved Hide resolved
src/app/page.tsx Outdated Show resolved Hide resolved
src/app/page.tsx Outdated Show resolved Hide resolved
@DevTrav
Copy link
Contributor Author

DevTrav commented Nov 13, 2024

Changes ready for review by @newswim

backgroundSize: 'cover',
backgroundPosition: 'center',
position: 'relative',
color: 'white',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the color here for the inner text elements? If so I would add color attributes to the specific element and not this parent.

{/* Overlay */}
<Stack
sx={{
position: 'absolute',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 seems a little bit odd... I'm not sure why the absolute positioning is being used here. Are you just wanting to center this element?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also seems odd to have a Stack without any child elements.. the whole point of Stack is to position child elements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants