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

Fixed Access the collection button style #978

Closed
wants to merge 4 commits into from

Conversation

Ankit-Gupta18
Copy link
Contributor

@Ankit-Gupta18 Ankit-Gupta18 commented Mar 17, 2022

Description

On the collection listing page, the style of the button to view a collection is broken. Some part of the label text is not visible because the background color doesn't cover the full button.

Rationale

Phabricator Ticket

//: # (Link to the Phabricator ticket)

How Has This Been Tested?

  1. Apply and approve an application to a partner.
  2. Add an expiration date in the admin view for that authorization
  3. Check that the access collection button is shown correctly when resizing your desktop screen.

Screenshots of your changes (if appropriate):

ss

Types of changes

What types of changes does your code introduce? Add an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Minor change (fix a typo, add a translation tag, add section to README, etc.)

@Ankit-Gupta18
Copy link
Contributor Author

Hi @suecarmol ,
Please review this.

Copy link
Contributor

@suecarmol suecarmol left a comment

Choose a reason for hiding this comment

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

Before I can review this, please fill out the How Has This Been Tested? and Screenshots of your changes (if appropriate): sections of the PR template

@Ankit-Gupta18
Copy link
Contributor Author

Before I can review this, please fill out the How Has This Been Tested? and Screenshots of your changes (if appropriate): sections of the PR template

I have added the screenshot but am quite confused about what to write in How Has This Been Tested?

@suecarmol
Copy link
Contributor

Before I can review this, please fill out the How Has This Been Tested? and Screenshots of your changes (if appropriate): sections of the PR template

I have added the screenshot but am quite confused about what to write in How Has This Been Tested?

You write the steps to check that the bug has been fixed. For example, in this case, you could add the following:

  1. Apply and approve an application to a partner.
  2. Add an expiration date in the admin view for that authorization
  3. Check that the access collection button is shown correctly when resizing your desktop screen.

Basically, the How Has This Been Tested? section is for adding instructions on how to manually test your PR

@Ankit-Gupta18
Copy link
Contributor Author

Hi @suecarmol ,
I have updated it. Please review it now.

@jsnshrmn
Copy link
Member

We had a problem with travis ci that was causing pr tests to fail. I mitigated the issue in master and rebased this PR to resolve the problem here too.

Copy link
Contributor

@suecarmol suecarmol left a comment

Choose a reason for hiding this comment

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

Hi @Ankit-Gupta18! Thank you for working on this bug fix. On taking a look at this PR, I think this is the wrong approach to try and solve this.

When I started developing this section, I took the wrong approach, and instead of working with Bootstrap 4 classes, I made my own breakpoints. I should've known not to re-invent the wheel :)

To really fix the bug, try looking at https://getbootstrap.com/docs/4.0/layout/grid/ to add a container div and column divs inside the card-footer section of TWLight/users/templates/users/user_collection_tile.html.

Let me know if you have any questions about how to approach this solution!

@Ankit-Gupta18
Copy link
Contributor Author

Hi @Ankit-Gupta18! Thank you for working on this bug fix. On taking a look at this PR, I think this is the wrong approach to try and solve this.

When I started developing this section, I took the wrong approach, and instead of working with Bootstrap 4 classes, I made my own breakpoints. I should've known not to re-invent the wheel :)

To really fix the bug, try looking at https://getbootstrap.com/docs/4.0/layout/grid/ to add a container div and column divs inside the card-footer section of TWLight/users/templates/users/user_collection_tile.html.

Let me know if you have any questions about how to approach this solution!

I tried adding container and col divs but it didn't work as expected. Possibly I must be placing them wrongly so, can you please detail to me where to exactly add those classes?

@suecarmol
Copy link
Contributor

Hi @Ankit-Gupta18! Thank you for working on this bug fix. On taking a look at this PR, I think this is the wrong approach to try and solve this.
When I started developing this section, I took the wrong approach, and instead of working with Bootstrap 4 classes, I made my own breakpoints. I should've known not to re-invent the wheel :)
To really fix the bug, try looking at https://getbootstrap.com/docs/4.0/layout/grid/ to add a container div and column divs inside the card-footer section of TWLight/users/templates/users/user_collection_tile.html.
Let me know if you have any questions about how to approach this solution!

I tried adding container and col divs but it didn't work as expected. Possibly I must be placing them wrongly so, can you please detail to me where to exactly add those classes?

Please upload your changes so I can take a look at them

@Ankit-Gupta18
Copy link
Contributor Author

Hi @Ankit-Gupta18! Thank you for working on this bug fix. On taking a look at this PR, I think this is the wrong approach to try and solve this.
When I started developing this section, I took the wrong approach, and instead of working with Bootstrap 4 classes, I made my own breakpoints. I should've known not to re-invent the wheel :)
To really fix the bug, try looking at https://getbootstrap.com/docs/4.0/layout/grid/ to add a container div and column divs inside the card-footer section of TWLight/users/templates/users/user_collection_tile.html.
Let me know if you have any questions about how to approach this solution!

I tried adding container and col divs but it didn't work as expected. Possibly I must be placing them wrongly so, can you please detail to me where to exactly add those classes?

Please upload your changes so I can take a look at them

I have uploaded them as a commit. It may contain blunders. Sorry for it but I am a bit confused at this part.

@Ankit-Gupta18
Copy link
Contributor Author

Hi @suecarmol ,
I have made the changes as you requested but I don't find it working as expected. I have uploaded the changes as a commit. Also attached my screenshot.
access

@suecarmol
Copy link
Contributor

Hi @suecarmol , I have made the changes as you requested but I don't find it working as expected. I have uploaded the changes as a commit. Also attached my screenshot. access

Try adding width: 98%; to the access-apply-button class and the renew-extend-button in new-local.css

@Ankit-Gupta18
Copy link
Contributor Author

Hi @suecarmol , I have made the changes as you requested but I don't find it working as expected. I have uploaded the changes as a commit. Also attached my screenshot. access

Try adding width: 98%; to the access-apply-button class and the renew-extend-button in new-local.css

Doing as you stated, things were not working as expected. I tried to add some CSS to make things look better(image attached). Please review them if it is fine.
access2

@suecarmol
Copy link
Contributor

Thank you for working on this! Because this was a trickier bug than I first thought, I tried fixing the bug myself and filed #987. You can take a look at how I solved this bug :)

@suecarmol suecarmol closed this Apr 9, 2022
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.

None yet

3 participants