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

Revamp profile visibility section #29482

Merged
merged 4 commits into from
Nov 4, 2021

Conversation

Pytal
Copy link
Member

@Pytal Pytal commented Oct 29, 2021

Revamp profile visibility settings by grouping dropdowns together into one section instead of one dropdown under every input field and make necessary improvements to the backend i.e. provide appId for sorting and dynamic displayId.

image
image

The design of the entire settings page is not final and this new section was placed in a separate block pending an overhaul of the layout @nextcloud/designers

Contributes to #28139

@Pytal Pytal added enhancement 2. developing Work in progress feature: settings feature: profile PRs or issues related to the Profile feature (e.g. Profile page, API, etc.) labels Oct 29, 2021
@Pytal Pytal added this to the Nextcloud 23 milestone Oct 29, 2021
@Pytal Pytal self-assigned this Oct 29, 2021
@Pytal Pytal mentioned this pull request Oct 29, 2021
20 tasks
@Pytal Pytal force-pushed the feat/revamp-profile-visibility-section branch 8 times, most recently from 027adfd to faff972 Compare October 30, 2021 03:37
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2021
19 tasks
@Pytal Pytal force-pushed the feat/revamp-profile-visibility-section branch from faff972 to 1d9d1a9 Compare November 2, 2021 01:39
@Pytal Pytal marked this pull request as ready for review November 2, 2021 01:39
@Pytal

This comment has been minimized.

@Pytal Pytal added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 2, 2021
@nextcloud-command nextcloud-command force-pushed the feat/revamp-profile-visibility-section branch from 1d9d1a9 to 88a77b3 Compare November 2, 2021 01:48
@Pytal Pytal requested review from a team, juliusknorr, skjnldsv, szaimen and CarlSchwan and removed request for a team November 2, 2021 01:50
@Pytal
Copy link
Member Author

Pytal commented Nov 2, 2021

Psalm errors seem unrelated 🤔

lib/private/Profile/ProfileManager.php Outdated Show resolved Hide resolved
lib/private/Profile/ProfileManager.php Outdated Show resolved Hide resolved
@CarlSchwan
Copy link
Member

Psalm errors seem unrelated thinking

If you can reproduce this error locally, could you update the psalm baseline?

Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

This looks so much better. Great work!

@skjnldsv
Copy link
Member

skjnldsv commented Nov 2, 2021

If you can reproduce this error locally, could you update the psalm baseline?

Let's do that in another PR

Comment on lines 5 to 11
html {
scroll-behavior: smooth;

@media screen and (prefers-reduced-motion: reduce) {
scroll-behavior: auto;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Seems super specific and new, but not related to this PR 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to make it so that clicking the "Edit your Profile visibility" anchor link scrolls smoothly to the new section instead of jumping instantly :)

Copy link
Member

Choose a reason for hiding this comment

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

I think that will impact everything else too then? Maybe just scope that to the visibility styling?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was already attempted to set this property on the scoped container which contains the elements, but in testing it seemed that setting on html was the only way it would work. As a js solution would be suboptimal I opted to place this in the styles of the component instead so that it would ony apply on the settings page with the component

<style lang="scss">
html {
scroll-behavior: smooth;
@media screen and (prefers-reduced-motion: reduce) {
scroll-behavior: auto;
}
}
</style>

Copy link
Contributor

@artonge artonge Nov 4, 2021

Choose a reason for hiding this comment

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

Indeed tricky as the scroll-behaviour has to be set on the scrolling element, and in this case, it is html.

You could wrap the personal settings into a <div>, limit its height to 100vh - $hearder-height and set the scroll-behaviour on it. But I didn't find how to include the development notice at the bottom.

In any case, setting scroll-behaviour globally could be a good change. But might need some discussion...

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, its merged already 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case, setting scroll-behaviour globally could be a good change. But might need some discussion...

Yes, would have to see if any current functionality depends on non-smooth scroll-behavior, though it'd most likely be purely cosmetic 🤔

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Some comments, otherwise good! 🚀

@Pytal Pytal force-pushed the feat/revamp-profile-visibility-section branch from 88a77b3 to 27fce8e Compare November 3, 2021 03:19
@Pytal
Copy link
Member Author

Pytal commented Nov 3, 2021

If you can reproduce this error locally, could you update the psalm baseline?

Let's do that in another PR

Pinging @https://github.com/nextcloud/server/blob/master/.github/workflows/update-psalm-baseline.yml :)

@Pytal Pytal requested a review from artonge November 3, 2021 03:30
@blizzz blizzz mentioned this pull request Nov 3, 2021
18 tasks
- Minor refactor

Signed-off-by: Christopher Ng <[email protected]>
Signed-off-by: Christopher Ng <[email protected]>
- Provide metadata
  - Dynamic displayId
  - Add appId
- Filter out unused parameter config properties from the existing profile config

Signed-off-by: Christopher Ng <[email protected]>
@Pytal Pytal force-pushed the feat/revamp-profile-visibility-section branch from 27fce8e to 8992eb2 Compare November 4, 2021 00:17
@Pytal
Copy link
Member Author

Pytal commented Nov 4, 2021

/compile amend /

Signed-off-by: Christopher Ng <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
@nextcloud-command nextcloud-command force-pushed the feat/revamp-profile-visibility-section branch from 8992eb2 to 19c62d0 Compare November 4, 2021 00:31
@skjnldsv skjnldsv merged commit 7bf9382 into master Nov 4, 2021
@skjnldsv skjnldsv deleted the feat/revamp-profile-visibility-section branch November 4, 2021 07:32
@Pytal Pytal added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 4, 2021
@Pytal Pytal mentioned this pull request Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: profile PRs or issues related to the Profile feature (e.g. Profile page, API, etc.) feature: settings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants