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.

Sign up for GitHub

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

Jump to bottom

feat(NcEmptyContent)!: make empty content centered by default instead of 20vh margin #4506

Merged
merged 2 commits into from Sep 8, 2023
Merged

feat(NcEmptyContent)!: make empty content centered by default instead of 20vh margin #4506

merged 2 commits into from Sep 8, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Sep 6, 2023 •
edited

☑️ Resolves

By default NcEmptyContent displays the content with fixed 20vh top margin. This doesn't look good sometimes. I think that could be useful to have centered NcEmptyContent.

For example, by default, it looks bad in comments, where the default margin is overridden now with !important.

Current !important override Default styles New centered
image image image

Or in Talk.

Large screen (almost centered, but not) Small screen (cut, but there is space)
image image

🖼️ Screenshots

🏚️ Before 🏡 After
image image

Also still works:

NcAppSidebar ReferencePicker
image image

🚧 Tasks

  • Remove margin-top: 20vh
  • Make content centered
  • Add flex: 1 to make it centered by default in flex parents
  • Remove unnecessary styles in other components that remove margin and check that nothing it breaking
    • NcHeaderMenu, NcDashboardWidget - custom styles, nothing breaking
    • NcAppSidebar - works fine by default because of flex
    • NcReferencePicker: NcProviderList, NcRawLinkInput, NcSearch - remove unnecessary margin: 0, works fine now by default

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@ShGKme ShGKme added enhancement New feature or request 3. to review Waiting for reviews feature: emptycontent Related to the emptycontent component labels Sep 6, 2023
@ShGKme ShGKme added this to the 8.0.0 milestone Sep 6, 2023
@ShGKme ShGKme self-assigned this Sep 6, 2023
@skjnldsv
Copy link
Contributor

skjnldsv commented Sep 6, 2023 •
edited

It was the old behaviour, but to be honest, maybe just make it centered straight away?
No prop? I think it make sense to stop using it with this weird top margin

@ShGKme ShGKme mentioned this pull request Sep 6, 2023
Merged
10 tasks
@ShGKme
Copy link
Contributor Author

ShGKme commented Sep 6, 2023

It was the old behaviour, but to be honest, maybe just make it centered straight away?
No prop? I think it make sense to stop using it with this weird top margin

We can, but it will be a breaking change.

Centered works well when component's user stretch it and the way it's been streatched may depends on a specific layout. For example, flex: 1 0; for flex container, height: 100% in some cased for element, in another for a its flex container and etc...

@jancborchardt
Copy link
Contributor

Agree with @skjnldsv that it should ideally be default instead of manually set prop. :)

@skjnldsv
Copy link
Contributor

skjnldsv commented Sep 6, 2023

We can, but it will be a breaking change.

v8 is still in beta and breaking, I think we can go there imho :)

@ShGKme ShGKme added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 7, 2023
@ShGKme ShGKme changed the title feat(NcEmptyContent): add centered prop feat(NcEmptyContent)!: make empty content centered by default instead of 20vh margin Sep 7, 2023
@ShGKme ShGKme added 3. to review Waiting for reviews breaking PR that requires a new major version and removed 2. developing Work in progress labels Sep 7, 2023
@ShGKme
Copy link
Contributor Author

ShGKme commented Sep 7, 2023

PR is updated. Now it is centered by default.

It works fine in all other nextcloud/vue components by default, I just removed unnecessary styles.

But it is a breaking change, in apps in parents without height and flex it will have not top margin now.

@ShGKme ShGKme closed this Sep 7, 2023
@ShGKme ShGKme reopened this Sep 7, 2023
@ShGKme
Copy link
Contributor Author

ShGKme commented Sep 7, 2023

Missclick ><

@ShGKme
feat(NcEmptyContent)!: make content centered without top margin
f969cab 
BREAKING CHANGE: there is no longer `margin-top: 20vh`.
You should ajust position by setting container height of using flex container.

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme
fix: adjust styles with new NcEmptyContent styles
a485280 
- Remove unnecessary margin:0 styles

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme requested a review from GretaD September 7, 2023 19:44
@skjnldsv skjnldsv merged commit ca177c2 into master Sep 8, 2023
16 checks passed
@skjnldsv skjnldsv deleted the feat/empty-content-centered branch September 8, 2023 06:50
@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented Sep 8, 2023 •
edited

/backport to next

Edit: I guess the backport bot is still dead.

@AndyScherzinger
Copy link
Contributor

Edit: I guess the backport bot is still dead.

Yes, we didn't find the time yet...

@Antreesy Antreesy mentioned this pull request Sep 14, 2023
Merged
@Pytal Pytal mentioned this pull request Sep 15, 2023
Merged
@ShGKme ShGKme mentioned this pull request Oct 10, 2023
Merged
61 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndyScherzinger AndyScherzinger AndyScherzinger approved these changes

@Pytal Pytal Pytal approved these changes

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

@marcoambrosini marcoambrosini Awaiting requested review from marcoambrosini

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@susnux susnux Awaiting requested review from susnux

@artonge artonge Awaiting requested review from artonge

@raimund-schluessler raimund-schluessler Awaiting requested review from raimund-schluessler

@GretaD GretaD Awaiting requested review from GretaD

Assignees

@ShGKme ShGKme

Labels
3. to review Waiting for reviews breaking PR that requires a new major version enhancement New feature or request feature: emptycontent Related to the emptycontent component
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@ShGKme @skjnldsv @jancborchardt @raimund-schluessler @AndyScherzinger @GretaD @Pytal