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

[FEATURE] make logo carousel linked logos same size as non linked logos. #409

Merged

Conversation

ghost
Copy link

@ghost ghost commented Apr 12, 2018

There is a size difference in the logo-carousel logos, when they are linked and when they are not.
Linked Logos are smaller compared to the not linked ones.

@ghost ghost closed this Apr 12, 2018
@ghost ghost deleted the bugfix-make-logo-carousell-link-icons-same-size branch April 12, 2018 13:40
@ghost ghost restored the bugfix-make-logo-carousell-link-icons-same-size branch April 12, 2018 13:41
@ghost
Copy link
Author

ghost commented Apr 12, 2018

accidentally deleted the wrong branch....

@ghost ghost reopened this Apr 12, 2018
@@ -7,10 +7,6 @@
padding: 0 30px;
}

.logo-carousel__slide {
padding: 10px 30px;
Copy link
Member

@dmh dmh Apr 20, 2018

Choose a reason for hiding this comment

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

I do not agree with this fix. If you remove padding from class .logo-carousel__slide it will be removed not only from the image without a link but also from the image with a link which causes problems in logo carousel element (logos will be too big)

Copy link
Author

Choose a reason for hiding this comment

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

At the moment the linked logos are smaller than the non linked logos.
This fixes the size difference.
We did not have any problems with to big logos.
Do you have an example so we can reproduce it?

Copy link
Member

Choose a reason for hiding this comment

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

padding: 10px 30px;
screen shot 2018-04-24 at 2 09 19 pm

- padding: 10px 30px;
screen shot 2018-04-24 at 2 08 49 pm

Copy link
Author

Choose a reason for hiding this comment

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

Here is a Screenshot of what I mean:
image

The Problem is that there is some css styling on the .logo-carousel__link class which means when there is no link set there is some styling missing.
The only visible difference is the padding.

I can either make the icons all the smaller size or the larger size, which one would you like?

Copy link
Member

Choose a reason for hiding this comment

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

My main concerns are that this fix can cause changes in existing websites after the update as I showed it on screenshots above. So you need to add changes without changing existed behavior. I would suggest making not linked files smaller.

@ghost
Copy link
Author

ghost commented May 28, 2018

I have updated the LogoCarousel.html and added a wrapper around the image when there is no link set and added the same styling to the wrapper .logo-carousel__wrapper class from the .logo-carousel__link.

So you need to add changes without changing existed behavior

Since this is a pull request of changes to the behavior it will change behavior...

Anyway I updated the PR to make non linked and linked logos the same size and styling without touching existing styles.

@dmh dmh merged commit 788496b into t3kit:master Jun 7, 2018
@websi websi deleted the bugfix-make-logo-carousell-link-icons-same-size branch June 29, 2018 14:34
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.

1 participant