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

Replace AppNavigationCounter with CounterBubble #1895

Merged
merged 3 commits into from
Apr 30, 2021

Conversation

PVince81
Copy link
Contributor

Because we need a more generic bubble, like in this example:
image
This is from nextcloud/spreed#5534

Keep the old file to not break existing imports.

I've tested it with some local Talk code that is using AppNavigationCounter and it properly redirects the component.

The only issue I see is that the style guide doesn't show the deprecation notice and replaces the component with the name:
image

@PVince81 PVince81 added the 2. developing Work in progress label Apr 29, 2021
@PVince81 PVince81 self-assigned this Apr 29, 2021
Replaces AppNavigationCounter as a more generic component.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Copy link
Contributor Author

the best way I found was keeping the old component as is and then marking as deprecated, it then appears like this in the style guide:
image

This would make it possible to backport to stable3 if necessary, then remove completely on master.

Thoughts ?

@PVince81 PVince81 changed the title Rename AppNavigationCounter to CounterBubble Replace AppNavigationCounter with CounterBubble Apr 29, 2021
@PVince81 PVince81 requested a review from skjnldsv April 29, 2021 12:49
@PVince81 PVince81 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 29, 2021
@PVince81 PVince81 added the enhancement New feature or request label Apr 29, 2021
@PVince81 PVince81 marked this pull request as ready for review April 29, 2021 12:50
@skjnldsv
Copy link
Contributor

Because we need a more generic bubble, like in this example:
image

This one seems a bit misaligned, no?

@PVince81
Copy link
Contributor Author

Because we need a more generic bubble, like in this example:
image

This one seems a bit misaligned, no?

it is, but not related to the component here.

I'll realign it with designer eyes later on in the Talk PR

@skjnldsv
Copy link
Contributor

But shouldn't this be staandardized here? So a bubble can be applied to an action?

@PVince81
Copy link
Contributor Author

But shouldn't this be staandardized here? So a bubble can be applied to an action?

Not sure how to standardize alignment in code. It would require knowing the size of the element (action, app navigation, ...) on top of which is needs to be positioned and aligned on top of it. Also need a proper container with position: relative for the positioning the work.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Copy link
Contributor Author

@raimund-schluessler thanks for pointing out. I've adjusted the usages and also fixed some unrelated warnings while browsing the ListItem page in the style guide.

After seeing the usages, maybe we could add a counter slot for the "Actions" element (or any of the relevent sub-elements), this way the alignment could be achieved by the action* components themselves.

<!--
- @copyright Copyright (c) 2019 Marco Ambrosini <marcoambrosini@pm.me>
-
- @author Marco Ambrosini <marcoambrosini@pm.me>
Copy link
Contributor

Choose a reason for hiding this comment

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

@PVince81 Probably not really important, but I guess this is the wrong year and author?

@skjnldsv skjnldsv merged commit b55c2df into master Apr 30, 2021
@skjnldsv skjnldsv deleted the enh/noid/counterbubble branch April 30, 2021 13:12
@skjnldsv skjnldsv mentioned this pull request Jun 4, 2021
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 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants