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: page header accessible when deleting webhook #6985

Merged
merged 5 commits into from
Sep 16, 2024

Conversation

harshit078
Copy link
Contributor

Description

This PR resolves the issue #6817

  • while deleting webhook, page header is still accessible for both mobile viewport and large devices
  • When creating a webhook, save or cancel state is not accessible due to page header being overflowed.
  • Setting state breaks spaces resulting due to max-width

Current Behaviour

Screen.Recording.2024-09-11.at.4.25.47.PM.mov
Screenshot 2024-09-11 at 4 23 51 PM Screenshot 2024-09-11 at 4 24 12 PM

Expected behavior

Screen.Recording.2024-08-31.at.7.09.35.PM.mov
Screenshot 2024-09-11 at 4 24 26 PM

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 accessibility issues with the PageHeader component, particularly for mobile viewports and when deleting webhooks.

  • Modified StyledTopBarContainer in PageHeader.tsx to adjust width and padding for mobile devices
  • Removed z-index property from StyledTopBarContainer, potentially affecting stacking order
  • Changes aim to keep header and buttons accessible during webhook deletion
  • Modifications should improve visibility of save/cancel buttons on mobile devices
  • Webhook-related files need review to ensure proper interaction with updated PageHeader component

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

Copy link
Contributor

@bosiraphael bosiraphael left a comment

Choose a reason for hiding this comment

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

Hello @harshit078, thank you for contributing !
I modified your PR to put back the z-index, this will be fixed in another PR: #6737 (comment)
Also, I changed the width of 93% to use 100% and changed the box-sizing to border-box.
There is still the issue that we are able to click on the buttons which are behind the overlay on mobile, this is likely due to a bug in useListenClickOutsideV2. I'll let @lucasbordeau take a look.

@bosiraphael bosiraphael merged commit b0c9b9e into twentyhq:main Sep 16, 2024
10 of 11 checks passed
Copy link

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 87C7:275B6C:1D817:37DAE:66E8553B.
    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%3Aharshit078%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'Mon, 16 Sep 2024 15:56:43 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'87C7:275B6C:1D817:37DAE:66E8553B'�[39m,
      �[32m'x-ratelimit-limit'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-remaining'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-reset'�[39m: �[32m'1726502263'�[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 87C7:275B6C:1D817:37DAE:66E8553B.'�[39m
    }
  },
  request: {
    method: �[32m'GET'�[39m,
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3Aharshit078%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.4 (linux; x64)'�[39m,
      authorization: �[32m'token [REDACTED]'�[39m
    },
    request: { hook: �[36m[Function: bound bound register]�[39m }
  }
}
danger-results://tmp/danger-results-44f7bf88.json

Generated by 🚫 dangerJS against 49ea1e7

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.

3 participants