AkkomaGang/akkoma-fe
4
13
Fork 70
Code Issues 84 Pull requests 17 Projects Releases 1 Wiki Activity

Optional widened main column #402

Merged
floatingghost merged 10 commits from Riedler/akkoma-fe:wide-columns-for-upstream into develop 2025-03-01 12:00:34 +00:00
Conversation 12 Commits 10 Files changed 9 +32 -6
Riedler commented 2024-06-16 15:05:42 +00:00
Contributor
Copy link

this necessitated some major layouting changes, so please tell me if there's anything broken that I didn't catch 😅

before after (off) after (on)
image image image

part of DNS#1

also, default is for this setting to be on, since I've personally heard only positive feedback for this change, but I wouldn't mind turning it to off if necessary

this necessitated some major layouting changes, so please tell me if there's anything broken that I didn't catch 😅 | before | after (off) | after (on) | | --- | --- | --- | | ![image](/attachments/7c806344-683d-45f3-90c4-1c725cfae6e9) | ![image](/attachments/23b8cf4a-a905-48aa-a758-f011c29d30b3) | ![image](/attachments/150bd427-76ca-4ecd-8570-20809bcea983) | part of [DNS#1](https://git.sakamoto.pl/domi/akkoma-fe/pulls/1) also, default is for this setting to be on, since I've personally heard only positive feedback for this change, but I wouldn't mind turning it to off if necessary
image.png
2.7 MiB
image.png
2.6 MiB
image.png
1.8 MiB
Riedler added 6 commits 2024-06-16 15:05:43 +00:00 
meta-comment: eliminated corner-case weirdness by replaced cursed CSS with slightly less cursed CSS
made wide column layout optional
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
4abddf5e6a
Oneric reviewed 2024-06-17 01:02:25 +00:00
Oneric left a comment
Member
Copy link

I don’t know CSS and the frontend specifics well enough to be able to tell which subtle breakages this might or might not bring. But one comment re what’s visible in the screenshots:

This apparently extends the underlay to always cover the whole screen rather than just the area with columns. Personally i have the underlay disabled anyway, but i’ve always assumed it is to distinguish the content area and possibly add some additional damping to the background and make posts more readable with translucent text backgrounds while still showing the background image in its original vibrancy in the padding area. If the underlay just applies to the whole area this purpose gets lost and one might as well just set a pre-mixed background colour or image

I don’t know CSS and the frontend specifics well enough to be able to tell which subtle breakages this might or might not bring. But one comment re what’s visible in the screenshots: This apparently extends the underlay to always cover the whole screen rather than just the area with columns. Personally i have the underlay disabled anyway, but i’ve always assumed it is to distinguish the content area and possibly add some additional damping to the background and make posts more readable with translucent text backgrounds while still showing the background image in its original vibrancy in the padding area. If the underlay just applies to the whole area this purpose gets lost and one might as well just set a pre-mixed background colour or image
Riedler commented 2024-06-17 09:20:17 +00:00
Author
Contributor
Copy link

I can change that somewhat easily I think, will do that in a bit

I can change that somewhat easily I think, will do that in a bit
Riedler added 2 commits 2024-06-26 14:44:05 +00:00
expand underlay to screen edges when TL is widened
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
2e2e87db75
Riedler commented 2024-06-26 14:44:40 +00:00
Author
Contributor
Copy link

the underlay problems should be fixed now

the underlay problems should be fixed now
Oneric commented 2024-06-30 21:53:02 +00:00
Member
Copy link

Thanks!

Seems to work fine for me in testing, but as mentioned before i’m not familiar enough with the frontend’s (and in general) CSS to give this a more thorough review

Thanks! Seems to work fine for me in testing, but as mentioned before i’m not familiar enough with the frontend’s (and in general) CSS to give this a more thorough review
Oneric reviewed 2024-06-30 21:55:01 +00:00
src/App.scss
@ -172,6 +172,10 @@ nav {
background-color: rgba(0, 0, 0, 0.15);
background-color: var(--underlay, rgba(0, 0, 0, 0.15));
Oneric commented 2024-06-30 21:55:01 +00:00
Member
Copy link

unrelated to your changes, but do you happen to know why background-color is defined twice here? iiuc the second parameter in var already is a fallback value. Is this for old browsers which don’ŧ support var at all, or something else?

unrelated to your changes, but do you happen to know why `background-color` is defined twice here? iiuc the second parameter in `var` already is a fallback value. Is this for old browsers which don’ŧ support `var` at all, or something else?
Riedler commented 2024-07-03 14:45:37 +00:00
Author
Contributor
Copy link

I would assume it's for older browers that don't know the var syntax, since invalid values get discarded, in which case line 172 would be in effect. At this point though, do we still want to support that? That's basically just for IE11 and other really really ancient browsers.

image
image source: https://caniuse.com/css-variables

I doubt akkoma works on these browsers at all tbh. If not for all of the grid and flex, definitely for our fairly modern javascript (though I'm less knowledgable there, so don't ask me for specifics xD).

I would assume it's for older browers that don't know the `var` syntax, since invalid values get discarded, in which case line 172 would be in effect. At this point though, do we still want to support that? That's basically just for IE11 and other really *really* ancient browsers. ![image](/attachments/b1aae1ef-c2b4-41b9-ab74-e7307b4f9ef6) image source: https://caniuse.com/css-variables I doubt akkoma works on these browsers at all tbh. If not for all of the grid and flex, definitely for our fairly modern javascript (though I'm less knowledgable there, so don't ask me for specifics xD).
image.png
303 KiB
👍 1
Riedler marked this conversation as resolved
Oneric commented 2024-06-30 22:07:23 +00:00
Member
Copy link

Actually there’s one thing: with this patch and wide mode turned off, the TL is slightly narrow than without the patch on non-wide (but not yet mobile) screens, e.g. 5:4

The difference is minute however and i only noticed in direct comparison, so imho nothing to worry about but documenting it here just in case

Actually there’s one thing: with this patch and wide mode turned off, the TL is _slightly_ narrow than without the patch on non-wide (but not yet mobile) screens, e.g. 5:4 The difference is minute however and i only noticed in direct comparison, so imho nothing to worry about but documenting it here just in case
Riedler commented 2024-07-03 14:47:36 +00:00
Author
Contributor
Copy link

hmm, the difference is probably due to the changes I made to padding, since the rest should be 1:1 the same as before in terms of width in non-wide mode.

hmm, the difference is probably due to the changes I made to padding, since the rest should be 1:1 the same as before in terms of width in non-wide mode.
Riedler commented 2025-01-13 16:51:05 +00:00
Author
Contributor
Copy link

ping - anything I should change to get this merged? Anything else I should do?

ping - anything I should change to get this merged? Anything else I should do?
Oneric commented 2025-01-14 13:12:06 +00:00
Member
Copy link

For both of your PRs: there are no more remarks/blockers from my side; it seemed fine to me in testing (assuming nothing broke after rebasing) but i’m also not an expert in JS and the frontend and couldn’t merge anyway `^^

For both of your PRs: there are no more remarks/blockers from my side; it seemed fine to me in testing (assuming nothing broke after rebasing) but i’m also not an expert in JS and the frontend and couldn’t merge anyway `^^
Riedler commented 2025-01-14 17:15:59 +00:00
Author
Contributor
Copy link

well, then who can merge? seems to me that the akkoma team needs more people in the fe department if a PR can be good and still lie dormant for months :/

well, then who can merge? seems to me that the akkoma team needs more people in the fe department if a PR can be good and still lie dormant for months :/
floatingghost commented 2025-01-14 19:55:53 +00:00
Owner
Copy link

if only we had a team..
I mean we have like, what, 2 or 3 common contributors and exactly none of them specialise in frontend :(

so
yeah it's sorta hard to manage the fe if I'm honest

if only we had a team.. I mean we have like, what, 2 or 3 common contributors and exactly none of them specialise in frontend :( so yeah it's sorta hard to manage the fe if I'm honest
Riedler commented 2025-01-14 19:57:44 +00:00
Author
Contributor
Copy link

Oh, I didn't know the situation is that dire. How does one become the akkoma frontend person?

Oh, I didn't know the situation is that dire. How does one become the akkoma frontend person?
floatingghost commented 2025-01-14 22:12:07 +00:00
Owner
Copy link

not sure if there's a formal process, but if someone did want to take on that sort of responsibility, naturally I would fully welcome it and appreciate it massively

I am... only semi-competent at frontend work. I can make a browser do what I want but not brilliantly - so do forgive me if my reviews on these aren't the best, but I'll go through them tomorrow and take a peek

not sure if there's a formal process, but if someone did want to take on that sort of responsibility, naturally I would fully welcome it and appreciate it massively I am... only semi-competent at frontend work. I can make a browser do what I want but not brilliantly - so do forgive me if my reviews on these aren't the best, but I'll go through them tomorrow and take a peek
❤️ 1
floatingghost reviewed 2025-01-15 21:45:28 +00:00
floatingghost left a comment
Owner
Copy link

it all works - one thing i did notice is that images are like, incomprehensively massive and take up a good 75% of the screen when this setting is applied, do you think it's worth scaling them down a little to make them a bit more balanced with the text?

pic related
image

it all works - one thing i did notice is that images are like, incomprehensively massive and take up a good 75% of the screen when this setting is applied, do you think it's worth scaling them down a little to make them a bit more balanced with the text? pic related [image](/attachments/fa6a9616-fcce-42e7-84f4-6ffe43f9180d)
image.png
330 KiB
src/modules/instance.js Outdated
@ -61,6 +61,7 @@ const defaultState = {
showNavShortcuts: true,
showWiderShortcuts: true,
sidebarRight: false,
widenTimeline: true,
floatingghost commented 2025-01-15 21:43:29 +00:00
Owner
Copy link

this should prooooobably be false by default i think

this should prooooobably be false by default i think
Riedler added 2 commits 2025-02-07 02:50:53 +00:00
made widenTimeline false by default
Some checks are pending
ci/woodpecker/pr/woodpecker Pipeline is pending approval
Details
ci/woodpecker/pull_request_closed/woodpecker Pipeline was successful
Details
f15b94d566
Riedler commented 2025-02-07 02:57:01 +00:00
Author
Contributor
Copy link

I reduced the maximum image height to a third of what it was before. What do you think?

(random posts for demonstration purposes)

before after
image image
image image

and the default for widenTimeline is false now 😔 no sneakily forcing my UI change onto everyone I guess /j

I reduced the maximum image height to a third of what it was before. What do you think? (random posts for demonstration purposes) |before|after| |--|--| |![image](/attachments/68c4084d-e9e7-44eb-954e-2671e085c20d)|![image](/attachments/4c37a0b4-ea2b-4850-be3f-9266895017f1)| |![image](/attachments/4c320605-8aea-459f-8ee6-abf5f998cb5a)|![image](/attachments/6433dead-8b69-477b-9e72-3753b4f1f5aa)| and the default for widenTimeline is false now 😔 no sneakily forcing my UI change onto everyone I guess /j
image.png
120 KiB
image.png
212 KiB
image.png
814 KiB
image.png
806 KiB
floatingghost merged commit 0bf9cb0660 into develop 2025-03-01 12:00:34 +00:00
Riedler deleted branch wide-columns-for-upstream 2025-03-01 13:11:28 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
a11y

Issues and PRs about accessibility

  
Bug

Applies to issues describing bugs

  
Bug fix

Pull requests that squash bugs

  
Critical Priority

   
Documentation

  
Feature

Pull requests that implement new features

  
Feature request

Issues that request new features

  
Held for next release cycle

PR reviewed during release candidate phase, approved and held as PR

  
High Priority

   
Low Priority

   
Medium Priority

   
Minor change

For minor changes, ie CSS alignment changes

  
Translation/Locale

For changes related to translations

  
WIP

Work in progress

No labels
a11y
Bug
Bug fix
Critical Priority
Documentation
Feature
Feature request
Held for next release cycle
High Priority
Low Priority
Medium Priority
Minor change
Translation/Locale
WIP
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: AkkomaGang/akkoma-fe#402
No description provided.
Delete branch "Riedler/akkoma-fe:wide-columns-for-upstream"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?