Skip to content

Fixes Improper padding in "Confused? Need Help?" modals #464 #465

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.

Sign up for GitHub

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

Jump to bottom
Merged
possumbilities merged 3 commits into from
Mar 31, 2023
Merged

Fixes Improper padding in "Confused? Need Help?" modals #464 #465

possumbilities merged 3 commits into from
Mar 31, 2023

Conversation

@shuence
Copy link
Contributor

@shuence shuence commented Mar 21, 2023 •
edited by Cronus1007
Loading

Fixes

Fixes #464 by @soustab10

Description

Fixed the Improper padding by changing padding in media queries.

Screenshots

Problem:

image

Solution:

image

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the default branch of the repository (main or master).
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added or updated tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

For the purposes of this DCO, "license" is equivalent to "license or public domain dedication," and "open source license" is equivalent to "open content license or public domain dedication."

Developer Certificate of Origin 
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@shuence shuence requested review from a team as code owners March 21, 2023 14:50
@shuence shuence requested review from annatuma, obulat and possumbilities and removed request for a team March 21, 2023 14:50
@soustab10
Copy link
Contributor

soustab10 commented Mar 21, 2023

@shuence Can you add screenshots of the same in desktop view?

possumbilities
possumbilities requested changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@possumbilities possumbilities left a comment

Choose a reason for hiding this comment

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

Well done, but I don't think the font-size needs to be reduced to accomplish this. I think the padding increase is enough. Please double check and consider removing the 15px font-size setting.

Thanks so much for the contribution! ❤️

@shuence
Copy link
Contributor Author

shuence commented Mar 22, 2023

@shuence Can you add screenshots of the same in desktop view?

There is no change in desktop view as i only changed the media query

@shuence
Copy link
Contributor Author

shuence commented Mar 22, 2023

Well done, but I don't think the font-size needs to be reduced to accomplish this. I think the padding increase is enough. Please double check and consider removing the 15px font-size setting.

Thanks so much for the contribution! ❤️

Actually When changed padding

image

This link get bigger and pushes the container to scroll rightwords.
So after adding font size 15px It also gets resolved.

image

@soustab10
Copy link
Contributor

soustab10 commented Mar 22, 2023

Well done, but I don't think the font-size needs to be reduced to accomplish this. I think the padding increase is enough. Please double check and consider removing the 15px font-size setting.

Thanks so much for the contribution! ❤️

Actually When changed padding

image

This link get bigger and pushes the container to scroll rightwords.

So after adding font size 15px It also gets resolved.

image

Play with overflow-x values and/or set the max-width of the text container the same as the modal width.

soustab10
soustab10 suggested changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@soustab10 soustab10 left a comment

Choose a reason for hiding this comment

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

Use overflow-x properties and set the max width of the text div same as the modal. Do not change font-size as it not required.

@shuence
Copy link
Contributor Author

shuence commented Mar 22, 2023

Well done, but I don't think the font-size needs to be reduced to accomplish this. I think the padding increase is enough. Please double check and consider removing the 15px font-size setting.

Thanks so much for the contribution! ❤️

I made the necessary changes as per your review

@possumbilities
Copy link
Contributor

possumbilities commented Mar 22, 2023

@shuence Can you add screenshots of the same in desktop view?

There is no change in desktop view as i only changed the media query

Yes, I meant adding it at all, even for the media query.

For the long links going outside the modal I'd suggest trying an approach like this: https://css-tricks.com/snippets/css/prevent-long-urls-from-breaking-out-of-container/

@shuence
Copy link
Contributor Author

shuence commented Mar 22, 2023

@shuence Can you add screenshots of the same in desktop view?

There is no change in desktop view as i only changed the media query

Yes, I meant adding it at all, even for the media query.

For the long links going outside the modal I'd suggest trying an approach like this: https://css-tricks.com/snippets/css/prevent-long-urls-from-breaking-out-of-container/

That's a different issue #463 Someone is working on it I think for this PR fixes #465 properly. I checked that the link seems fine on 99% of the mobile devices without any change

image

@possumbilities possumbilities merged commit ccff0eb into creativecommons:main Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Cronus1007 Cronus1007 Cronus1007 approved these changes

@possumbilities possumbilities possumbilities approved these changes

@annatuma annatuma Awaiting requested review from annatuma annatuma is a code owner automatically assigned from creativecommons/technology

@obulat obulat Awaiting requested review from obulat obulat is a code owner automatically assigned from creativecommons/ct-license-chooser-maintainers

+1 more reviewer

@soustab10 soustab10 soustab10 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improper padding in "Confused? Need Help?" modals

4 participants

@shuence @soustab10 @possumbilities @Cronus1007