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

Disabled themes screenshots wrap funny #1047

Closed
docwilmot opened this issue Jul 2, 2015 · 16 comments · Fixed by backdrop/backdrop#3239
Closed

Disabled themes screenshots wrap funny #1047

docwilmot opened this issue Jul 2, 2015 · 16 comments · Fixed by backdrop/backdrop#3239

Comments

@docwilmot
Copy link
Contributor

docwilmot commented Jul 2, 2015

@wesruv
Same as #988
disabledthemeswrapfunny


PR: backdrop/backdrop#2853

@wesruv
Copy link
Member

wesruv commented Jul 2, 2015

Thanks @docwilmot!

@wesruv
Copy link
Member

wesruv commented Jul 5, 2015

PR Ready for this one: backdrop/backdrop#991

@quicksketch
Copy link
Member

Yikes, it's been a long time since this was filed. I found a few issues in the PR. @wesruv, do you think you could revisit this issue?

@ghost
Copy link

ghost commented Sep 6, 2019

Here's an updated PR that addresses @quicksketch's issues.

I found that the 'fix' for flex-wrap actually caused my disabled themes to display 4 to a line (instead of 3 like before the PR) which caused a horizontal scrollbar. So I commented out that line which fixed it, but this'll either need removing or replacing with a different solution if that issue is still relavent for other browsers...

@klonos
Copy link
Member

klonos commented Jun 11, 2020

@BWPanda care to refresh the PR sandbox?

@ghost
Copy link

ghost commented Jun 11, 2020

@klonos Done

@klonos
Copy link
Member

klonos commented Jun 12, 2020

Thanks @BWPanda 👍 ...looks much better now:

screencapture-2853-backdrop-backdrop-qa-backdropcms-org-admin-appearance-2020-06-13-01_18_20

screencapture-2853-backdrop-backdrop-qa-backdropcms-org-admin-appearance-2020-06-13-01_18_48

@klonos
Copy link
Member

klonos commented Jun 12, 2020

...wondering if it'd look better on narrow screens (when they are rendered in a single column) if the theme title/version, description, and "Enable" links we placed beside the theme thumbnails instead of bellow them:

screencapture-2853-backdrop-backdrop-qa-backdropcms-org-admin-appearance-2020-06-13-01_19_26

That way we'd put the white space to good use, and avoid too much scrolling when many themes are installed 😉

I've also left some feedback in the PR.

@ghost
Copy link

ghost commented Aug 12, 2020

I can no longer replicate this issue on the latest Backdrop 1.x... Can anyone else? If so, how?

@stpaultim
Copy link
Member

I was able to recreate the problem, but in a manner that might not be realistic and applicable.

I used the Chrome inspector to triple the length of the description for the Basis contrib theme and this broke the layout.

Appearance___Simplo

@stpaultim
Copy link
Member

stpaultim commented Aug 13, 2020

OK, I got it to happen without any manipulation. The description for the Teamwork 15 theme goes onto 4 lines and this is apparently enough to block the next row of themes from floating to the left.

Appearance___Simplo

@ghost
Copy link

ghost commented Aug 13, 2020

Based on the feedback from @klonos and @docwilmot, I made a new, simpler PR that I think fixes this, without a bunch of unnecessary, duplicated code: backdrop/backdrop#3239

@docwilmot
Copy link
Contributor Author

Tested locally, works well. RTBC for me, although I am not great at CSS.

@ghost
Copy link

ghost commented Aug 15, 2020

Will wait for another review then before I commit this.

@stpaultim
Copy link
Member

Well, I also tested this locally and it worked for me. I reviewed the code and don't see any problems. But, I'm also not very familiar with css practices/standards in Backdrop CMS core.

@ghost
Copy link

ghost commented Aug 16, 2020

Thanks for filing this @docwilmot (and sorry for the delay)! Thank you @wesruv for the original PR on which the fix was based, and to @quicksketch, @klonos & @stpaultim for testing and feedback. I've merged backdrop/backdrop#3239 into 1.x and 1.16.x.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants