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

Update Mobile Navigation bar css for better usage #6209

Merged
merged 4 commits into from
Jul 11, 2024

Conversation

dhruvdabhi101
Copy link
Contributor

Closes #5744

Description

  • Made Navigation Bar fixed in Mobile screens.
  • Added margin-bottom so, last entries doesn't get hidden.
Screenshot 2024-07-10 at 11 58 50 PM Screenshot 2024-07-10 at 11 59 06 PM

Copy link

@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

  • Made NavigationBar fixed at the bottom on mobile (/packages/twenty-front/src/modules/ui/navigation/navigation-bar/components/NavigationBar.tsx)
  • Added background color to NavigationBar for better visibility on mobile (/packages/twenty-front/src/modules/ui/navigation/navigation-bar/components/NavigationBar.tsx)
  • Introduced bottom margin to RecordTable on mobile to prevent last entries from being hidden (/packages/twenty-front/src/modules/object-record/record-table/components/RecordTable.tsx)
  • Utilized useIsMobile hook to conditionally apply styles (/packages/twenty-front/src/modules/object-record/record-table/components/RecordTable.tsx)

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

Copy link

github-actions bot commented Jul 10, 2024

Fails
🚫

node failed.

Log

�[31mError: �[39m RequestError [HttpError]: You have exceeded a secondary rate limit. Please wait a few minutes before you try again. If you reach out to GitHub Support for help, please include the request ID 2407:3556B9:75D64DA:D782A55:668F78C9.
    at /home/runner/work/twenty/twenty/node_modules/�[4m@octokit�[24m/request/dist-node/index.js:86:21
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m {
  status: �[33m403�[39m,
  response: {
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3Adhruvdabhi101%20is%3Aclosed%20repo%3Atwentyhq%2Ftwenty&per_page=2&page=1'�[39m,
    status: �[33m403�[39m,
    headers: {
      �[32m'access-control-allow-origin'�[39m: �[32m'*'�[39m,
      �[32m'access-control-expose-headers'�[39m: �[32m'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset'�[39m,
      connection: �[32m'close'�[39m,
      �[32m'content-encoding'�[39m: �[32m'gzip'�[39m,
      �[32m'content-security-policy'�[39m: �[32m"default-src 'none'"�[39m,
      �[32m'content-type'�[39m: �[32m'application/json; charset=utf-8'�[39m,
      date: �[32m'Thu, 11 Jul 2024 06:16:41 GMT'�[39m,
      �[32m'referrer-policy'�[39m: �[32m'origin-when-cross-origin, strict-origin-when-cross-origin'�[39m,
      server: �[32m'github.com'�[39m,
      �[32m'strict-transport-security'�[39m: �[32m'max-age=31536000; includeSubdomains; preload'�[39m,
      �[32m'transfer-encoding'�[39m: �[32m'chunked'�[39m,
      vary: �[32m'Accept-Encoding, Accept, X-Requested-With'�[39m,
      �[32m'x-content-type-options'�[39m: �[32m'nosniff'�[39m,
      �[32m'x-frame-options'�[39m: �[32m'deny'�[39m,
      �[32m'x-github-api-version-selected'�[39m: �[32m'2022-11-28'�[39m,
      �[32m'x-github-media-type'�[39m: �[32m'github.v3; format=json'�[39m,
      �[32m'x-github-request-id'�[39m: �[32m'2407:3556B9:75D64DA:D782A55:668F78C9'�[39m,
      �[32m'x-ratelimit-limit'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-remaining'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-reset'�[39m: �[32m'1720678661'�[39m,
      �[32m'x-ratelimit-resource'�[39m: �[32m'search'�[39m,
      �[32m'x-ratelimit-used'�[39m: �[32m'1'�[39m,
      �[32m'x-xss-protection'�[39m: �[32m'0'�[39m
    },
    data: {
      documentation_url: �[32m'https://docs.github.com/free-pro-team@latest/rest/overview/rate-limits-for-the-rest-api#about-secondary-rate-limits'�[39m,
      message: �[32m'You have exceeded a secondary rate limit. Please wait a few minutes before you try again. If you reach out to GitHub Support for help, please include the request ID 2407:3556B9:75D64DA:D782A55:668F78C9.'�[39m
    }
  },
  request: {
    method: �[32m'GET'�[39m,
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3Adhruvdabhi101%20is%3Aclosed%20repo%3Atwentyhq%2Ftwenty&per_page=2&page=1'�[39m,
    headers: {
      accept: �[32m'application/vnd.github.v3+json'�[39m,
      �[32m'user-agent'�[39m: �[32m'octokit-rest.js/18.12.0 octokit-core.js/3.6.0 Node.js/18.20.3 (linux; x64)'�[39m,
      authorization: �[32m'token [REDACTED]'�[39m
    },
    request: { hook: �[36m[Function: bound bound register]�[39m }
  }
}
danger-results:https://tmp/danger-results-f095259e.json

Generated by 🚫 dangerJS against 11deddb

@FelixMalfait
Copy link
Member

Thanks for the contribution! I don't think that's the issue, it's probably this instead: https://www.reddit.com/r/css/comments/12mh0ac/safari_mobile_does_not_respect_height_100vh/ ; I will make a quick fix

Copy link

@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

(updates since last review)

  • Removed useIsMobile hook and mobile-specific margin adjustment in /packages/twenty-front/src/modules/object-record/record-table/components/RecordTable.tsx
  • Removed fixed positioning and background color from StyledContainer in /packages/twenty-front/src/modules/ui/navigation/navigation-bar/components/NavigationBar.tsx
  • Added margin-bottom to NavigationBar to prevent last entries from being hidden on mobile

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

@FelixMalfait
Copy link
Member

@Bonapara fyi changing most vh to dvh in the app.
https://www.alsacreations.com/astuce/lire/1831-corriger-le-probleme-de-hauteur-100vh-sur-mobile.html (old article before dvh)

Copy link

@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

(updates since last review)

  • Updated height property from 100vh to 100dvh in multiple files for better mobile viewport handling (/packages/twenty-front/src/loading/components/LeftPanelSkeletonLoader.tsx, /packages/twenty-front/src/loading/components/UserOrMetadataLoader.tsx, /packages/twenty-front/src/modules/ui/feedback/dialog-manager/components/Dialog.tsx, /packages/twenty-front/src/modules/ui/layout/page/BlankLayout.tsx, /packages/twenty-front/src/modules/ui/layout/page/DefaultLayout.tsx, /packages/twenty-front/src/pages/auth/Authorize.tsx)
  • Reordered import statements in /packages/twenty-front/src/loading/components/LeftPanelSkeletonLoader.tsx and /packages/twenty-front/src/modules/ui/layout/page/DefaultLayout.tsx for better code organization

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

@FelixMalfait FelixMalfait merged commit 45fe537 into twentyhq:main Jul 11, 2024
8 checks passed
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.

Bottom Bar Not Visible on Mobile Without Scrolling
2 participants