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

Create modal window for contacts settings #3032

Merged
merged 1 commit into from
Oct 14, 2022

Conversation

JuliaKirschenheuter
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter commented Oct 11, 2022

Resolves #2952

After Before
image Screenshot from 2022-10-11 14-19-10

@JuliaKirschenheuter JuliaKirschenheuter self-assigned this Oct 11, 2022
@JuliaKirschenheuter JuliaKirschenheuter added the 2. developing Work in progress label Oct 11, 2022
@JuliaKirschenheuter JuliaKirschenheuter marked this pull request as draft October 11, 2022 13:00
@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

Base: 31.41% // Head: 31.41% // No change to project coverage 👍

Coverage data is based on head (41ee662) compared to base (31cb306).
Patch coverage: 0.00% of modified lines in pull request are covered.

❗ Current head 41ee662 differs from pull request most recent head bd536c0. Consider uploading reports for the commit bd536c0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #3032   +/-   ##
=========================================
  Coverage     31.41%   31.41%           
  Complexity      253      253           
=========================================
  Files           110      110           
  Lines          1862     1862           
  Branches        218      217    -1     
=========================================
  Hits            585      585           
  Misses         1162     1162           
  Partials        115      115           
Impacted Files Coverage Δ
src/components/AppNavigation/ContactsSettings.vue 0.00% <0.00%> (ø)
src/components/AppNavigation/RootNavigation.vue 0.00% <ø> (ø)
...nts/AppNavigation/Settings/SettingsAddressbook.vue 0.00% <ø> (ø)
src/components/ContactDetails.vue 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JuliaKirschenheuter JuliaKirschenheuter added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 11, 2022
@JuliaKirschenheuter JuliaKirschenheuter marked this pull request as ready for review October 11, 2022 13:33
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the enh/2952-move_settings_to_a_modal branch from b269d19 to 81508ad Compare October 11, 2022 14:03
@szaimen
Copy link
Contributor

szaimen commented Oct 11, 2022

I just tested this and it seems to work. 🚀

Only the sort option may not work as expected currently but this may be an already existing issue. (changing sorting between the speicifc options sometimes does not change the contacts order even though it should theoratically be changed in order to reflect the new setting).


Also, can we somehow remove the strange looking border betweeen the input field and the arrow for creating the new address book?

image


Also, can we make it more clear when an address book is disabled by addding (disaabled) behind its name then?
really ugly mockup:
image


Also, the share button should probably switch to a different icon when a calendar is alredy shared? (and the icon is moving down when the share view is expanded)
image


additionally, importing contacts is very easily to break e.g. by choosing a local non-ics file for import or choosing from files and clicking on the close button of the file picker

@ChristophWurst
Copy link
Member

One step at a time please @szaimen. Let's move the settings in this PR and keep the redesign for a follow-up.

@szaimen
Copy link
Contributor

szaimen commented Oct 12, 2022

One step at a time please @szaimen. Let's move the settings in this PR and keep the redesign for a follow-up.

Fine by me. I'll create a follow-up ticket

@szaimen
Copy link
Contributor

szaimen commented Oct 12, 2022

Done in #3033

Copy link
Contributor

@szaimen szaimen 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 works but didn't review the code

@JuliaKirschenheuter
Copy link
Contributor Author

Done in #3033

Thank you @szaimen for the valuable feedback! I agree with your points and will work on them asap. Yesterday it was a bit urgent to finish a first state and follow-ups will be done in a next days.

@ChristophWurst
Copy link
Member

@JuliaKirschenheuter are there any blockers for the setting sections?

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 great

@ChristophWurst ChristophWurst added enhancement New feature or request 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 14, 2022
@ChristophWurst
Copy link
Member

Please squash

Signed-off-by: julia.kirschenheuter <julia.kirschenheuter@nextcloud.com>
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the enh/2952-move_settings_to_a_modal branch from 41ee662 to bd536c0 Compare October 14, 2022 07:47
@ChristophWurst ChristophWurst merged commit d739722 into main Oct 14, 2022
@ChristophWurst ChristophWurst deleted the enh/2952-move_settings_to_a_modal branch October 14, 2022 07:49
@st3iny st3iny added this to the v5.1.0 milestone Feb 2, 2023
@GretaD GretaD mentioned this pull request Apr 18, 2023
73 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Left sidebar: Move settings to a modal
4 participants