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

Fix counter margin #1912

Merged
merged 1 commit into from
May 6, 2021
Merged

Conversation

marcoambrosini
Copy link
Contributor

@marcoambrosini marcoambrosini commented May 3, 2021

fix #1868

Bringing back only the counter fix.

Signed-off-by: Marco Ambrosini [email protected]

@marcoambrosini marcoambrosini added bug Something isn't working 3. to review Waiting for reviews regression Regression of a previous working feature labels May 3, 2021
@marcoambrosini marcoambrosini added this to the 4.0.0 milestone May 3, 2021
@marcoambrosini marcoambrosini self-assigned this May 3, 2021
Copy link
Contributor

@raimund-schluessler raimund-schluessler left a comment

Choose a reason for hiding this comment

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

Actions now miss the right margin:

Screenshot_2021-05-03 Nextcloud Vue Style Guide

@marcoambrosini marcoambrosini force-pushed the fix-appnavigationitem-counter-2 branch from bc91cd8 to 7bda7f7 Compare May 3, 2021 09:43
@marcoambrosini
Copy link
Contributor Author

Actions now miss the right margin

🤦 done 😅

Copy link
Contributor

@raimund-schluessler raimund-schluessler left a comment

Choose a reason for hiding this comment

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

Looks good now.

Screenshot_2021-05-03 Nextcloud Vue Style Guide(1)

@raimund-schluessler
Copy link
Contributor

The fix for the Calendar app will still be necessary, but I guess that's inevitable and can be fixed quickly in the Calendar app.

Copy link
Contributor

@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.

That's a breaker though, not really a fix

@skjnldsv
Copy link
Contributor

skjnldsv commented May 3, 2021

If we need to fix something for it to work in another app, that's not ok.
This needs to be backportable to stable3, meaning there should be no other change needed on the app than bumping @nextcloud/vue's version :)

@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented May 3, 2021

If we need to fix something for it to work in another app, that's not ok.
This needs to be backportable to stable3, meaning there should be no other change needed on the app than bumping @nextcloud/vue's version :)

Technically, yes. But using the counter slot like Tasks and Calendar do it, was never really supported by nextcloud/vue. So I am a bit undecided if we need to account for it.

In the Tasks app, I already included the necessary fix:
https://github.com/nextcloud/tasks/blob/master/src/components/AppNavigation/ListItemCalendar.vue#L583-L585

So, I don't mind whatever is decided.

@skjnldsv
Copy link
Contributor

skjnldsv commented May 3, 2021

The question is, does it works in any of 3.x.x of nextcloud/vue?
If so I assume following proper semver means we need to have this fixed properly?

@raimund-schluessler
Copy link
Contributor

The question is, does it works in any of 3.x.x of nextcloud/vue?
If so I assume following proper semver means we need to have this fixed properly?

Yes, it does work in 3.x.x. If we want to not break this usage, we could either try to not introduce this wrapper around the CounterBubble or maybe apply this rule to it:

.app-navigation-entry__counter-wrapper {
    margin-right: 2px;
    display: flex;
    align-items: center;
    flex: 0 1 auto;
}

This should also keep it working, I think (didn't test it yet, can do it only in the evening).

@skjnldsv
Copy link
Contributor

skjnldsv commented May 3, 2021

Why do we need this wrapper at all btw?
I'm also ok adding the wrapper for 4.x.x but only the margin for 3.x.x @ma12-co :)

@marcoambrosini
Copy link
Contributor Author

marcoambrosini commented May 6, 2021

Why do we need this wrapper at all btw?

To apply the extra margin without using v-deep

Signed-off-by: Marco Ambrosini <[email protected]>
@marcoambrosini marcoambrosini force-pushed the fix-appnavigationitem-counter-2 branch from 7bda7f7 to 64f88ef Compare May 6, 2021 09:45
@marcoambrosini
Copy link
Contributor Author

Applyed flex rules to solve calendar issue

Screenshot from 2021-05-06 10-45-04

@marcoambrosini marcoambrosini merged commit c3fe45b into master May 6, 2021
@marcoambrosini marcoambrosini deleted the fix-appnavigationitem-counter-2 branch May 6, 2021 09:57
@marcoambrosini
Copy link
Contributor Author

/backport to stable3

@raimund-schluessler
Copy link
Contributor

I can confirm it now works without adjustments, nextcloud/tasks#1564.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working regression Regression of a previous working feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AppNavigation counter have wrong right margin
3 participants