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

Make container view looks proportional on viewport #1700

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

solracsf
Copy link
Member

@solracsf solracsf commented Jun 6, 2024

On big screen resolution, actual implementation with fixed with looks like an iframe.
Make it fluid and align with other items on screen.

Before

image

After

image

@solracsf solracsf requested review from ShGKme and susnux June 6, 2024 07:06
@solracsf solracsf added this to the Nextcloud 30 milestone Jun 6, 2024
@solracsf
Copy link
Member Author

solracsf commented Jun 6, 2024

/backport to stable29

@solracsf
Copy link
Member Author

solracsf commented Jun 6, 2024

/backport to stable28

Signed-off-by: Git'Fellow <[email protected]>
Copy link

cypress bot commented Jun 6, 2024

1 flaky test on run #1584 ↗︎

0 10 0 0 Flakiness 1

Details:

Make container view looks proportional on viewport
Project: Activity Commit: fcd0b5e0b3
Status: Passed Duration: 02:20 💡
Started: Jun 10, 2024 5:41 PM Ended: Jun 10, 2024 5:43 PM
Flakiness  cypress/e2e/sidebar.cy.ts • 1 flaky test • Run E2E

View Output

Test Artifacts
Check activity listing in the sidebar > Has tag activity Test Replay Screenshots

Review all test suite changes for PR #1700 ↗︎

margin: 0 auto;
padding-inline: 12px;
// Align with app navigation toggle
margin: var(--app-navigation-padding, 8px) 0 0 calc(2 * var(--app-navigation-padding, 8px) + 44px);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
margin: var(--app-navigation-padding, 8px) 0 0 calc(2 * var(--app-navigation-padding, 8px) + 44px);
margin: var(--app-navigation-padding, 8px) 0 0 calc(2 * var(--app-navigation-padding, 8px) + var(--default-clickable-area));

Copy link
Contributor

Choose a reason for hiding this comment

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

But we should also add a variable or component for that to not calculate it all the time in each app...

I'll do it later today.

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem; same code a few lines down.

padding-inline: 12px;
// Align with app navigation toggle
margin: var(--app-navigation-padding, 8px) 0 0 calc(2 * var(--app-navigation-padding, 8px) + 44px);
padding-inline: 0 44px;
Copy link
Contributor

Choose a reason for hiding this comment

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

If this padding is defined by the a button size

Suggested change
padding-inline: 0 44px;
padding-inline: 0 var(--default-clickable-area);

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

This was done on purpose to make the list look like a list, as on bigger screens it becomes unreadable otherwise.
Same layout like e.g. on forms or others.

So the list is centered and fixed width. But I agree the scrollbar currently looks not good and should be moved to the very right instead directly next to the centered list.

With this changes it looks really weird and unreadable on wide screens, so I would request a review of @nextcloud/designers

before with this
Screenshot 2024-06-06 at 11-10-53 Aktivität - Aquarium Screenshot 2024-06-06 at 11-11-46 Aktivität - Aquarium

@solracsf
Copy link
Member Author

solracsf commented Jun 6, 2024

With this changes it looks really weird and unreadable on wide screens

Well, we think the same but with current implementation 😄

I think the problem is that the date is floating right of the activity line. If they all (activity and date) were aligned left, i think this would be the best option.

Should this be validated, i could work on that if no one wants to do that.

But, a centered and fixed width doesn't seem the best option IMHO.

@susnux
Copy link
Contributor

susnux commented Jun 6, 2024

But, a centered and fixed width doesn't seem the best option IMHO.

That's why I cc the designers. We use the same layout also in Talk, Announcement Center, and Forms so basically all apps that are "lists" content wise

@ShGKme
Copy link
Contributor

ShGKme commented Jun 6, 2024

That's why I cc the designers. We use the same layout also in Talk, Announcement Center, and Forms so basically all apps that are "lists" content wise

Not really the same. Styles are local here and it looks different. For example, we don't have a scroll bar in the middle of content in Talk. It's on the corner.

@ShGKme
Copy link
Contributor

ShGKme commented Jun 6, 2024

What about adding a light border between items, so it's easy to connect time and item visually?

@solracsf
Copy link
Member Author

solracsf commented Jun 6, 2024

@ShGKme please feel free to take the lead on this one 👍

@susnux
Copy link
Contributor

susnux commented Jun 6, 2024

For example, we don't have a scroll bar in the middle of content in Talk. It's on the corner.

Yes as I wrote this is a bug currently but the limited width is on design purpose (see also current settings changes).

the scrollbar currently looks not good and should be moved to the very right instead directly next to the centered list.

Signed-off-by: Grigorii K. Shartsev <[email protected]>
@ShGKme
Copy link
Contributor

ShGKme commented Jun 10, 2024

@solracsf @susnux I've pushed a commit with an intermediate change:

  • Scroll bar pushed to the page's edge
  • The content is kept on the left (similar to most of the apps like Photos, Settings)
  • Using more variables

But I'd make it even wide than in the past

Before This PR Proposal
image image image

@marcoambrosini
Copy link
Member

marcoambrosini commented Jun 11, 2024

Width should be 900px. If possible NcAppSettingsSection should be user. Your proposal looks good @ShGKme

@ShGKme
Copy link
Contributor

ShGKme commented Jun 11, 2024

If possible NcAppSettingsSection should be user

I'd not use NcAppSettingsSection even if we want a similar design... NcAppSettingsSection by definition has a different purpose. We should not think about activities and other usages while modifying settings components.

We can just use the same design here or introduce a new component.

@marcoambrosini
Copy link
Member

We should not think about activities and other usages while modifying settings components

Many already do... Do you think that then it would make sense to create an ad hoc component there that all app developers can use?

@ShGKme
Copy link
Contributor

ShGKme commented Jun 11, 2024

@marcoambrosini Do you mean NcAppSettingsSection or NcSettingsSection?

Many already do...

Do you have an example?

Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Fully agree with @susnux here, the scrollbar hanging next to the text is not by design and it should be all the way on the right.

@ShGKme your proposal looks good, but the content should still be visually centered. Especially on bigger screens the left-alignment looks strange. (Something we need to fix in Settings as well.)

@marcoambrosini
Copy link
Member

Something we need to fix in Settings as well.

@jancborchardt settings should be left aligned as the sidebar is a integral part of the content and on a big screen they get too separated. That is unless we're able to center the navigation too

See the following screenshot as a reference. Granted: it's not great, but think about if the main content was centered
Screenshot 2024-08-19 at 15 47 43

@skjnldsv skjnldsv mentioned this pull request Aug 22, 2024
44 tasks
@blizzz blizzz mentioned this pull request Aug 29, 2024
24 tasks
@solracsf
Copy link
Member Author

@ShGKme your proposal looks good, but the content should still be visually centered. Especially on bigger screens the left-alignment looks strange.

IMO, the centered content looks strange, as everything else is left aligned... 😄

@ShGKme
Copy link
Contributor

ShGKme commented Aug 29, 2024

@jancborchardt @marcoambrosini

This PR is open for quite a long time, and comments from the design team are different.

Are you against this open (settings like, limited width on the left with scroll bar on the right)?

image

@marcoambrosini
Copy link
Member

@ShGKme my comment was only concerning settings. I would prefer if there we kept the left alignment

@jancborchardt
Copy link
Member

@ShGKme so to clarify, for Activity:

  • Limited width as before (can be wider, would be nice if it is the same as in Talk, Text etc)
  • Centered as before
  • Scrollbar on right edge

@jancborchardt
Copy link
Member

Additionally actually: The heading "All activities" (or the others) would also look better if it’s left aligned with the centered content, instead of all the way on the left.

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

Successfully merging this pull request may close these issues.

5 participants