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

4.0.0 #2007

Merged
merged 1 commit into from
Jun 10, 2021
Merged

4.0.0 #2007

merged 1 commit into from
Jun 10, 2021

Conversation

skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented Jun 4, 2021 •
edited
Loading

v4.0.0 (2021-06-04)

Full Changelog

Breaking

Enhancements

Fixed bugs

@skjnldsv skjnldsv added the 2. developing Work in progress label Jun 4, 2021
@skjnldsv skjnldsv added this to the 4.0.0 milestone Jun 4, 2021
@skjnldsv

This comment has been minimized.

Sign in to view
@skjnldsv skjnldsv added the breaking PR that requires a new major version label Jun 4, 2021
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 4, 2021
@skjnldsv skjnldsv requested a review from nickvergessen June 4, 2021 13:33
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 4, 2021
korelstar
korelstar previously requested changes Jun 5, 2021
View reviewed changes
Copy link
Contributor

@korelstar korelstar left a comment

Choose a reason for hiding this comment

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

I found two design issues in 4.0.0 (see screenshots below):

  • AppNavigationCaption is aligned right. According to Update appnavigationcaption #1863, this is not correct.
  • AppNavigationItem's edit mode has no vissible cancel button.

nc-vue 3.10.1

Screenshot before

nc-vue 4.0.0

Screenshot after

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Jun 5, 2021

@ma12-co ?

@marcoambrosini
Copy link
Contributor

@korelstar the component looks ok in the documentation, are you sure this is not due to custom css you have in your app? I'll test this with talk too

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Jun 7, 2021

Rebased, let's see documentation

@marcoambrosini
Copy link
Contributor

Rebased, let's see documentation

Looks good

@skjnldsv skjnldsv requested a review from korelstar June 7, 2021 11:06
PVince81
PVince81 reviewed Jun 7, 2021
View reviewed changes
CHANGELOG.md
@@ -1,6 +1,54 @@
# Changelog

All notable changes to this project will be documented in this file.
## [v4.0.0](https://github.com/nextcloud/nextcloud-vue/tree/v4.0.0) (2021-06-04)
Copy link
Contributor

Choose a reason for hiding this comment

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

change to 2021-06-07 before merging ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@korelstar
Copy link
Contributor

korelstar commented Jun 7, 2021 •
edited
Loading

@korelstar the component looks ok in the documentation, are you sure this is not due to custom css you have in your app? I'll test this with talk too

You can reproduce the first issue (AppNavigationCaption is aligned right) in the docs by removing the ActionButton from the example.

Screenshot

You can reproduce the second issue (editable AppNavigationItem) in the docs by changing the width of the app-navigation-entry container to 300px like in a real app-navigation.

Screnshot

Did you ever tested the changes in a real app?

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Jun 7, 2021

Did you ever tested the changes in a real app?

@korelstar
Are you speaking globally about this pr or just this specific use-case?

@korelstar
Copy link
Contributor

I was just wondering about the reaction that everything is looking fine. 😉

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Jun 7, 2021

Not everyone uses edit in the appNavigation

@korelstar
Copy link
Contributor

Sorry, that was not meant as affront. Maybe we can surround all AppNavigationItem examples in the docs with <ul style="width: 300px">? That would make the examples more realistic.

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Jun 8, 2021

Sorry, that was not meant as affront

Not taken as is :) 🤗

@skjnldsv
Copy link
Contributor Author

Seems ok to me, LGTM

300px:
image

Normal on docs:
image

@skjnldsv
Copy link
Contributor Author

image
image

@skjnldsv
Copy link
Contributor Author

Caption fix #2024

@skjnldsv skjnldsv merged commit d2a7d54 into master Jun 10, 2021
@skjnldsv skjnldsv deleted the npm/v4.0.0 branch June 10, 2021 07:34
@skjnldsv skjnldsv restored the npm/v4.0.0 branch June 10, 2021 07:35
@korelstar korelstar mentioned this pull request Jun 11, 2021
Merged
@skjnldsv skjnldsv deleted the npm/v4.0.0 branch June 11, 2021 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 left review comments

@marcoambrosini marcoambrosini marcoambrosini approved these changes

@GretaD GretaD GretaD approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

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

@artonge artonge Awaiting requested review from artonge

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@korelstar korelstar Awaiting requested review from korelstar

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish breaking PR that requires a new major version
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@skjnldsv @marcoambrosini @korelstar @PVince81 @ChristophWurst @GretaD