Skip to content

Conversation

@hamza221
Copy link
Contributor

@hamza221 hamza221 commented Dec 6, 2023

Composer state : I couldn't replicate the unwrapped state we had with the counter

1 2 3
image image image

@hamza221 hamza221 self-assigned this Dec 6, 2023
@hamza221 hamza221 added 2. developing dependencies skill:frontend Issues and PRs that require JavaScript/Vue/styling development skills labels Dec 6, 2023
@ChristophWurst
Copy link
Member

For a reviewable upgrade, I would suggest to clean up our code from removed components while staying on the old nc/vue, then bump the lib and do the rest of the changes.

Something like

  1. A PR against main to replace NcMultiselect with NcSelect
  2. A PR against main to migrate NcPopoverMenu
  3. ...
  4. Bump nc/vue in package.json

https://github.com/nextcloud-libraries/nextcloud-vue/blob/master/CHANGELOG.md#boom-breaking-changes

@GretaD
Copy link
Contributor

GretaD commented Dec 7, 2023

For a reviewable upgrade, I would suggest to clean up our code from removed components while staying on the old nc/vue, then bump the lib and do the rest of the changes.

Something like

  1. A PR against main to replace NcMultiselect with NcSelect

i have started that one: its almost done, need to rebase it and thats it think: #8814

@hamza221 hamza221 force-pushed the enh/bump-nextcloud-vue branch 2 times, most recently from cf98fc5 to bcbccb6 Compare December 7, 2023 14:48
@hamza221 hamza221 requested a review from GretaD December 7, 2023 15:12
@GretaD
Copy link
Contributor

GretaD commented Dec 7, 2023

Lets merge this one and close mine #8814
This way we avoid more work from hamza to resolve conflicts

@hamza221
Copy link
Contributor Author

hamza221 commented Dec 7, 2023

  • The only know issue is composer ncSelects, working on fixing it

@GretaD
Copy link
Contributor

GretaD commented Dec 21, 2023

2 small bugs:

Screenshot from 2023-12-21 15-48-16
Screenshot from 2023-12-21 15-47-46

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks sane but didn't test yet

Any strong reason for aliasing NcSelect to SelectVue?

.mail-empty-content {
margin: 0 auto;
}
.empty-content {
Copy link
Member

Choose a reason for hiding this comment

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

nit: BEM and getting rid of the mail-empty-content container

@hamza221
Copy link
Contributor Author

hamza221 commented Dec 22, 2023

Any strong reason for aliasing NcSelect to SelectVue?

Select tag already exists in Html, and Not NcSelect just because we're already aliasing other components, so to keep it consistent

@ChristophWurst
Copy link
Member

For consistency with other apps and the upstream name I would prefer NcSelect

@ChristophWurst
Copy link
Member

The only know issue is composer ncSelects, working on fixing it

still to do?

@hamza221
Copy link
Contributor Author

hamza221 commented Jan 8, 2024

still to do?

Done, but I couldn't mimic the old behaviour tho, might still need improvements.
edit: new behaviour in screenshots in the description.

@ChristophWurst
Copy link
Member

ChristophWurst commented Jan 9, 2024

  • BUG: Half a NcSelect

image

@ChristophWurst
Copy link
Member

ChristophWurst commented Jan 9, 2024

  • BUG: Envelopes with primary color looks a bit odd. Contrast is definitely problematic.

image

@nimishavijay
Copy link
Member

Looks good in the screenshots, just not sure about this weird space on the left, the avatar should have equal spacing on the left, top and bottom
image

@GretaD
Copy link
Contributor

GretaD commented Jan 9, 2024

Bug

  • I Cannot close the Account Settings with the close button or clicking outside. I can close it only with esc
  • Create calendar and create task - the calendar names are not shown:

Screenshot from 2024-01-09 17-05-40

@hamza221
Copy link
Contributor Author

image cc @nimishavijay

@GretaD GretaD force-pushed the enh/bump-nextcloud-vue branch from 0057758 to 7fd347d Compare January 17, 2024 18:22
Copy link
Contributor

@GretaD GretaD left a comment

Choose a reason for hiding this comment

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

tested and rebased, changed 2 small things

@GretaD GretaD force-pushed the enh/bump-nextcloud-vue branch from 7fd347d to cb10622 Compare January 17, 2024 18:30
@ChristophWurst
Copy link
Member

ChristophWurst commented Jan 18, 2024

#9144 (comment) is still there and a blocker

Bildschirmfoto vom 2024-01-18 09-36-24

edit: seems fixed. I was not on the latest commit.

@ChristophWurst
Copy link
Member

  • BUG primary color for active mailboxes
Before After
Bildschirmfoto vom 2024-01-18 09-37-31 Bildschirmfoto vom 2024-01-18 09-37-13

@ChristophWurst

This comment was marked as resolved.

@ChristophWurst

This comment was marked as resolved.

@nimishavijay
Copy link
Member

image

Much better!

  • There is still unequal spacing on the left, top and bottom of the avatar, they should all be equal
  • close button should also have a couple of pixels more space on the right

@GretaD
Copy link
Contributor

GretaD commented Jan 18, 2024

  • BUG primary color for active mailboxes

Before After
Bildschirmfoto vom 2024-01-18 09-37-31 Bildschirmfoto vom 2024-01-18 09-37-13

this comes from an index.css that overlaps the active state of app-navigation as well as the envelope list. :deep and !important on the component doesnt make any difference.
Screenshot from 2024-01-18 11-16-46

@ChristophWurst
Copy link
Member

If the color is coming from nc/vue for any active element then I assume this must have a purpose. Let's keep it but make the text readable.

@GretaD
Copy link
Contributor

GretaD commented Jan 19, 2024

the issue with text on mail was because we force the color on our app. I removed that. The new design is white on active state. See talk as well.
Screenshot from 2024-01-19 16-47-55

@GretaD GretaD force-pushed the enh/bump-nextcloud-vue branch from 4339dd7 to deb9626 Compare January 19, 2024 15:51
@ChristophWurst
Copy link
Member

ChristophWurst commented Jan 19, 2024

  • BUG
[Vue warn]: Invalid prop: custom validator check failed for prop "additionalTrapElements".

found in

---> <NcDialog>
       <NcAppSettingsDialog>
         <AccountSettings> at src/components/AccountSettings.vue
           <RouterLink>
             <NcAppNavigationItem>
               <NavigationAccount> at src/components/NavigationAccount.vue
                 <NcAppNavigation>
                   <Navigation> at src/components/Navigation.vue
                     <NcContent>
                       <Home> at src/views/Home.vue
                         <App> at src/App.vue
                           <Root> vue.runtime.esm.js:4609

@GretaD
Copy link
Contributor

GretaD commented Jan 23, 2024

closed accidentally, because it was mentioned as a fix on the nc/vue pr. sorry about that :)

@GretaD GretaD added the blocked label Jan 23, 2024
@GretaD GretaD force-pushed the enh/bump-nextcloud-vue branch from 356727d to 21cdaf3 Compare January 24, 2024 09:31
@GretaD GretaD removed the blocked label Jan 24, 2024
@GretaD GretaD force-pushed the enh/bump-nextcloud-vue branch from 21cdaf3 to fd2bdc4 Compare January 24, 2024 14:20
@GretaD
Copy link
Contributor

GretaD commented Jan 24, 2024

i tested all what i could. The PR is huge, im sure we will miss something anyway. The most commonly used actions work fine.
If someone has the patience, please test, otherwise, lets merge it :)

Signed-off-by: hamza mahjoubi <[email protected]>
@GretaD GretaD force-pushed the enh/bump-nextcloud-vue branch from 8a2ffd3 to de995ed Compare January 25, 2024 09:56
@GretaD GretaD merged commit 4ee6682 into main Jan 25, 2024
@GretaD GretaD deleted the enh/bump-nextcloud-vue branch January 25, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review dependencies skill:frontend Issues and PRs that require JavaScript/Vue/styling development skills

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants