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(GUI) Stretched Logo on Mobile and Safari #86

Conversation

nitro56565
Copy link
Contributor

@nitro56565 nitro56565 commented Jul 17, 2024

Closes #84 #82

Description

This pull request addresses issue #84 , which highlights the problem of the logo appearing stretched on mobile devices and in the Safari browser. The issue was caused by browser-defined CSS, which has now been fixed. The logo will maintain its aspect ratio and display correctly on all devices and browsers, including mobile devices and Safari.

Changes

  • Ensured the logo's dimensions are set to auto to maintain its aspect ratio.
  • Used CSS media queries to adjust the logo size for different screen sizes.
  • Tested and fixed Safari-specific CSS issues.

Screenshots or Video

safari-before
safari-after
safari-mobile-before
safari-mobile-after

Related Issues

Author Checklist

  • Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
  • Vital features and changes captured in unit and/or integration tests
  • Commits messages follow AP format
  • Extend the documentation, if necessary
  • Merging to main from fork:branchname

@nitro56565 nitro56565 requested a review from a team as a code owner July 17, 2024 18:25
Copy link

netlify bot commented Jul 17, 2024

Deploy Preview for ap-template-playground ready!

Name Link
🔨 Latest commit fbcb79b
🔍 Latest deploy log https://app.netlify.com/sites/ap-template-playground/deploys/669a565e0a28ad0008a1f9b4
😎 Deploy Preview https://deploy-preview-86--ap-template-playground.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@nitro56565 nitro56565 changed the title Fix Stretched Logo on Mobile and Safari (#84) Fix(GUI) Stretched Logo on Mobile and Safari Jul 17, 2024
@nitro56565
Copy link
Contributor Author

Hi @Vinyl-Davyl can you please review the code

@Vinyl-Davyl
Copy link
Collaborator

Hi @Vinyl-Davyl can you please review the code

Hi @nitro56565 looks great.
Can you ensure you sign the DCO Checks?

Signed-off-by: Mahesh Patil <nitro56565@gmail.com>
Signed-off-by: Mahesh Patil <nitro56565@gmail.com>
@nitro56565 nitro56565 force-pushed the nitro56565/i84/safari-header-logo-bug branch from 77d5208 to d9258e5 Compare July 18, 2024 15:23
@nitro56565
Copy link
Contributor Author

@Vinyl-Davyl done

1 similar comment
@nitro56565
Copy link
Contributor Author

@Vinyl-Davyl done

Copy link
Collaborator

@Vinyl-Davyl Vinyl-Davyl left a comment

Choose a reason for hiding this comment

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

changes and fixes for this have been made(i.e ConcertoEditor) in the most recent PR before this.

could you try reviewing the PR before this, see where it’s being made so you could make adjustments to avoid front running merge conflicts.

@nitro56565
Copy link
Contributor Author

alright I'll check that

Signed-off-by: Mahesh Patil <nitro56565@gmail.com>
@nitro56565
Copy link
Contributor Author

I have merged your pr for concerto editor please check and let me know if there is any issue.

@@ -105,7 +105,7 @@ function Navbar({ scrollToExplore }: { scrollToExplore: any }) {
preview={false}
style={{
paddingRight: screens.md ? "1.5em" : "10px",
height: "26px",
height: "26px", maxWidth: screens.md ? '184.17px' : '36.67px',
Copy link
Member

Choose a reason for hiding this comment

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

This is fine for now. @Vinyl-Davyl @nitro56565 can we please look into having a consistent unit for styles, we are using em, px both. Let's please fix this as well.

@sanketshevkar
Copy link
Member

@nitro56565 what is the concerto editor change about?

@@ -105,7 +105,7 @@ function Navbar({ scrollToExplore }: { scrollToExplore: any }) {
preview={false}
style={{
paddingRight: screens.md ? "1.5em" : "10px",
Copy link
Collaborator

Choose a reason for hiding this comment

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

okay @nitro56565 can you change this: 1.5em in the paddingRight, to the equivalent of 24 pixels from the base format 16px.

so change 1.5em to 24px to ensure consistency here!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright I'll do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Vinyl-Davyl done with these changes

@nitro56565
Copy link
Contributor Author

@sanketshevkar there was type declaration issue in concerto which I fixed because my build was failing, but @Vinyl-Davyl had already fixed it in his new PR therefore I matched his changes with mine to avoid merge conflict.

Signed-off-by: Mahesh Patil <nitro56565@gmail.com>
@sanketshevkar
Copy link
Member

@sanketshevkar there was type declaration issue in concerto which I fixed because my build was failing, but @Vinyl-Davyl had already fixed it in his new PR therefore I matched his changes with mine to avoid merge conflict.

Are @Vinyl-Davyl changes already merged? If yes, you had to rebase to the main. We shouldn't be seeing the diff for ConcertoEditor.

…and encoded template data in URLs (accordproject#85)

* chore: updated spinner style

Signed-off-by: Vinyl-Davyl <okononfuadavid@gmail.com>

* feat: functionality to generate shareable links that include the encoded template data with lz-string implementation

Signed-off-by: Vinyl-Davyl <okononfuadavid@gmail.com>

* chore: concertoEditor build fixes

Signed-off-by: Vinyl-Davyl <okononfuadavid@gmail.com>

* chore: build fixes

Signed-off-by: Vinyl-Davyl <okononfuadavid@gmail.com>

* fix: review changes

Signed-off-by: Vinyl-Davyl <okononfuadavid@gmail.com>

* feat: URL versioning

Signed-off-by: Vinyl-Davyl <okononfuadavid@gmail.com>

---------

Signed-off-by: Vinyl-Davyl <okononfuadavid@gmail.com>
@nitro56565
Copy link
Contributor Author

@sanketshevkar there was type declaration issue in concerto which I fixed because my build was failing, but @Vinyl-Davyl had already fixed it in his new PR therefore I matched his changes with mine to avoid merge conflict.

Are @Vinyl-Davyl changes already merged? If yes, you had to rebase to the main. We shouldn't be seeing the diff for ConcertoEditor.

His changes are not yet merged should I rebase it back to the last changes as it was? but it would create a build error will that be fine?

Signed-off-by: Mahesh Patil <nitro56565@gmail.com>
Signed-off-by: Mahesh Patil <nitro56565@gmail.com>
Signed-off-by: Mahesh Patil <nitro56565@gmail.com>
Signed-off-by: Mahesh Patil <nitro56565@gmail.com>
@nitro56565
Copy link
Contributor Author

Hey @Vinyl-Davyl I have merged your PR which was just pushed on the main branch with mine the code looks fine at my end can you please review. @sanketshevkar

@Vinyl-Davyl
Copy link
Collaborator

Vinyl-Davyl commented Jul 19, 2024

@sanketshevkar there was type declaration issue in concerto which I fixed because my build was failing, but @Vinyl-Davyl had already fixed it in his new PR therefore I matched his changes with mine to avoid merge conflict.

Are @Vinyl-Davyl changes already merged? If yes, you had to rebase to the main. We shouldn't be seeing the diff for ConcertoEditor.

His changes are not yet merged should I rebase it back to the last changes as it was? but it would create a build error will that be fine?

Hi @nitro56565 the changes has been merged now. I think you should rebase to main / pull the latest changes to ensure sync and eliminate the blocker you have on build. Also you should scope the changes to the "Stretched Logo on Mobile and Safari" issue i.e the Navbar.tsx file alone now, then you should be good to go.

@nitro56565 nitro56565 closed this Jul 19, 2024
@nitro56565 nitro56565 deleted the nitro56565/i84/safari-header-logo-bug branch July 19, 2024 17:38
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.

Stretched logo on mobile and safari
3 participants