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

Add adaptive width style #28

Merged

Conversation

LemonIllusion
Copy link
Contributor

@LemonIllusion LemonIllusion commented Aug 3, 2019

There was a request to make articles as wide as the forum. I have had this functionality in my fork for a while, but because of a tricky condition that I didn't bother fixing, I never submitted a PR for it... until now! The condition is still not fixed, but maybe @SimonAlling has a good idea of how to either figure the entire condition out or make an acceptable workaround.

If anyone wants to try it out without building themselves, I've provided a testing build below. If you have any feedback you can comment here or @ me in the official Better SweClockers thread.

Install testing build beta.1

Testing build built from a5f7f00

@LemonIllusion
Copy link
Contributor Author

Beta.2

  • Move instead of remove rightmost ad
  • Cosmetic change to code: comments above related blocks in CSS

Install testing build beta.2

Testing build built from 7cef88a

src/operations.ts Outdated Show resolved Hide resolved
src/operations.ts Outdated Show resolved Hide resolved
@LemonIllusion
Copy link
Contributor Author

LemonIllusion commented Aug 4, 2019

.pushColumn>.fixed blir tom om man kryssar ur tillräckligt mycket i innehållsfiltret. Just nu får man dessutom köra extra CSS för att bli av med SweC 20 år. Det ser rätt konstigt ut att inte göra något åt den tomma ytan och i halv skärm (som är en av de främsta anledningarna från mitt håll att göra en sådan här stil över huvud taget) vill jag verkligen inte bli begränsad till en minikolumn för nyheterna när det finns plats att utnyttja precis bredvid. Tyvärr kan jag inte komma på ett bra sätt att göra det här med enbart CSS.


.pushColumn finns på bland annat:

  • Profilsida
  • Marknadsannons
  • Sök i arkivet
  • Framsida
  • Artiklar

Den saknas på bland annat:

  • Alla olika sorters undersidor till forumet
  • Gallerier
  • Marknad
  • Guider

Villkoret blir väldigt komplicerat oavsett om man gör white- eller blacklist, så jag hade helst kört en "går det så går det"-lösning som misslyckas i tystnad på sidor som saknar .pushColumn.

En annan lösning skulle kunna vara att bara låta .pushColumn>.fixed kunna försvinna på framsidan. Jag ska testa och se om det är något jag kan vara nöjd med.

@LemonIllusion LemonIllusion force-pushed the feature/adaptive-width branch 2 times, most recently from 527985a to 61e9cac Compare August 4, 2019 12:34
@LemonIllusion
Copy link
Contributor Author

Maybe I should just have the operation in my own fork where I don't really care about having errors in the console. I'm probably the only one who makes use of it anyway. Anyway, I made some changes, let's see where they take us.

Beta.3

  • Use limited whitelist for .pushColumn>.fixed removal
    It won't work everywhere now, but at least the two most important cases, front page and market listings are working. I could live with this as most other pages where it happens are uncommon to visit (example)
  • Fix image in "Guider" not being full width
  • Fix description of operation to fit grammatically

Install testing build beta.3

Testing build built from 61e9cac

@SimonAlling
Copy link
Owner

SimonAlling commented Aug 4, 2019

The one thing I'm completely sure about is that you should not keep this feature to your fork, because it's a very nice feature. :) I think it might be possible to do it with CSS only; otherwise, I think the best solution is to decide whether to apply the style based on the path – i.e. something like adaptiveWidthStyleMakesSense in environment.ts.

EDIT: I had not checked out the Beta 3 code when I wrote this comment.

EDIT 2: There seems to be a lot of weird float and padding stuff going on; maybe we can rewrite the base layout to interact better with the adaptive width feature.

@SimonAlling
Copy link
Owner

.pushColumn>.fixed blir tom om man kryssar ur tillräckligt mycket i innehållsfiltret. Just nu får man dessutom köra extra CSS för att bli av med SweC 20 år. Det ser rätt konstigt ut att inte göra något åt den tomma ytan och i halv skärm (som är en av de främsta anledningarna från mitt håll att göra en sådan här stil över huvud taget) vill jag verkligen inte bli begränsad till en minikolumn för nyheterna när det finns plats att utnyttja precis bredvid. Tyvärr kan jag inte komma på ett bra sätt att göra det här med enbart CSS.

Tack, bra förklarat!

.pushColumn finns på bland annat:

  • Profilsida
  • Marknadsannons
  • Sök i arkivet
  • Framsida
  • Artiklar

Den saknas på bland annat:

  • Alla olika sorters undersidor till forumet
  • Gallerier
  • Marknad
  • Guider

Villkoret blir väldigt komplicerat oavsett om man gör white- eller blacklist, så jag hade helst kört en "går det så går det"-lösning som misslyckas i tystnad på sidor som saknar .pushColumn.

En annan lösning skulle kunna vara att bara låta .pushColumn>.fixed kunna försvinna på framsidan. Jag ska testa och se om det är något jag kan vara nöjd med.

Vi vill dock inte gärna göra den här typen av modifieringar med DOM-operationer, eftersom det innebär att sidan laddas in och sedan ändras bredden på huvudinnehållet. Det ser inte bra ut och är mycket irriterande i synnerhet på långsammare uppkopplingar. Vi bör göra allt för att lösa detta med CSS.

Ska för övrigt nämna att vi ej vill röra .pushColumn på sidor som /arkiv, då den är essentiell för funktionaliteten där.

@LemonIllusion
Copy link
Contributor Author

Fan vad jag älskar att du är envis! Jag skrev om kolumndelen med flex och strösslade med lite magi och det ser faktiskt ut som att det fungerar med bara CSS nu. Jag vågar inte påstå att jag har någon susning om varför eller hur det funkar, men det kan man klura på någon annan dag.

Beta.4

  • Flex, magic and tiny bit of luck
  • Piped the DOM operation to /dev/null
  • Things might break in new hilarious ways ←feature
  • By accidentdefinitely on purpose, .pushColumn>.fixed is no longer fixed, but I like it better this way (until someone points out where it breaks badly)
    Keep an eye on if it grows beyond what it was before (154px inner width, 162px outer width)

Install testing build beta.4

Testing build built from b6674e2

@LemonIllusion
Copy link
Contributor Author

LemonIllusion commented Aug 4, 2019

First bug: /arkiv, "Mest kommenterade" doesn't fit.

Another bug: images on /arkiv should have width: 100%

@LemonIllusion
Copy link
Contributor Author

No new bugs have been reported and the new CSS looks like it's working well. Let's get this wheel spinning for real now.

RC.1

  • Fixed width on .pushColumn>.fixed, it was fun while it lasted
  • Rectified image width on /arkiv

Install release candidate 1

built from b277532

@LemonIllusion LemonIllusion marked this pull request as ready for review August 9, 2019 19:43
@LemonIllusion
Copy link
Contributor Author

LemonIllusion commented Aug 9, 2019

Största frågetecknet från min sida är var rutan i inställningarna ska vara. BETA-texten tar jag bort när jag flyttar den till rätt ställe.

@SimonAlling
Copy link
Owner

Tycker den ska ligga under Allmänt (general), precis efter Kompakt layout. Övriga kommentarer och åtgärder får nog vänta till efter Qontinent.

@SimonAlling
Copy link
Owner

SimonAlling commented Aug 16, 2019

There are two more things I would like to bring up before merging:

  • I think the option name should be more concise. What about LemonIllusions adaptiva layout?
  • The adaptive layout doesn't work very well together with Bättre rättelsegränssnitt. (You can see this by hitting Skicka en rättelse to bring up the corrections interface.) However, I don't think this is a major issue, because most people don't submit a lot of corrections, and either of the options can be disabled anyway. Ergo, you don't have to fix this for the PR to be merged.

@LemonIllusion
Copy link
Contributor Author

The positioning CSS for the Bättre rättelsegränssnitt looked a bit funky to me and the transform makes the entire box misaligned with the pixel grid resulting in the entire box getting blurry. I will suggest changes in a separate PR and make it work better with the adaptive layout as well.

@SimonAlling SimonAlling merged commit 70139b6 into SimonAlling:develop Aug 17, 2019
@LemonIllusion LemonIllusion deleted the feature/adaptive-width branch August 17, 2019 20:51
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

2 participants