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

fix #7085 breadcrumb fix for mobile viewport #7419

Merged

Conversation

NitinPSingh
Copy link
Contributor

ISSUE
Closes #7085

DEMO

03.10.2024_20.15.32_REC.mp4

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request addresses the issue of header overflow in the Data Model settings for mobile viewports. The changes focus on improving the Breadcrumb component for better mobile navigation.

  • Modified Breadcrumb.tsx to provide a simplified mobile-specific rendering with a back button
  • Added PagesRedirectToApp.ts to define pages that should redirect to the main app on mobile
  • Implemented conditional rendering in Breadcrumb.tsx based on device type (mobile or desktop)
  • Enhanced mobile UX by showing either "Back to App" or "Back to [previous page]" for easier navigation
  • Utilized useIsMobile hook to determine the device type and adjust the breadcrumb display accordingly

2 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings

Comment on lines 53 to 54
currentPage as string,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Casting 'currentPage' to string may cause runtime errors if it's a ReactNode. Consider using a type guard instead.

</StyledLink>
</Fragment>
) : (
<StyledText title={''}>{previousLink?.children}</StyledText>
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Empty title attribute here. Consider removing it or providing a meaningful title.

@lucasbordeau lucasbordeau self-assigned this Oct 10, 2024
@lucasbordeau lucasbordeau added the blocked: design needed This doesn't seem right label Oct 10, 2024
@lucasbordeau
Copy link
Contributor

@Bonapara Could you provide a more detailed explanation of the flow on mobile, because right now it seems odd

@Bonapara
Copy link
Member

The idea is to have a "Back button" that returns to the previous step. What seemed odd during testing?

@Bonapara Bonapara removed the blocked: design needed This doesn't seem right label Oct 17, 2024
@lucasbordeau
Copy link
Contributor

@Bonapara
image

For me it seems like "Back to XYZ" isn't relevant here as we would want to just go back to the settings menu. We would like to have the same effect as when clicking on the cog.

@lucasbordeau lucasbordeau added the blocked: design needed This doesn't seem right label Oct 17, 2024
@Bonapara
Copy link
Member

Let's do that @lucasbordeau, great idea!

@charlesBochet
Copy link
Member

@lucasbordeau let's merge this one tomorrow!

@charlesBochet
Copy link
Member

/award 150

Copy link

oss-gg bot commented Oct 31, 2024

Awarding NitinPSingh: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/NitinPSingh

@lucasbordeau lucasbordeau enabled auto-merge (squash) October 31, 2024 17:04
@lucasbordeau lucasbordeau merged commit e5641c5 into twentyhq:main Oct 31, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Header overflows in Data model for mobile viewport
4 participants