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

Add RoomChangePermissionsScreen #2513

Merged
merged 2 commits into from
Mar 1, 2024
Merged

Add RoomChangePermissionsScreen #2513

merged 2 commits into from
Mar 1, 2024

Conversation

pixlwave
Copy link
Member

@pixlwave pixlwave commented Feb 29, 2024

First part of #2358 this PR adds the screen and theoretically the VM implementation, but it isn't part of a flow yet, so is untested against a real room.

Screenshot 2024-02-28 at 11 52 54 am

@pixlwave pixlwave requested a review from a team as a code owner February 29, 2024 16:24
@pixlwave pixlwave requested review from Velin92 and removed request for a team February 29, 2024 16:24
Copy link

github-actions bot commented Feb 29, 2024

Warnings
⚠️ Some of the commits are missing ticket numbers. Please consider squashing all commits that don't have a tracking number.
⚠️ You seem to have made changes to views. Please consider adding screenshots.

Generated by 🚫 Danger Swift against 7b8817a

@pixlwave pixlwave force-pushed the doug/change-permissions branch from d1120bd to 1dd13de Compare February 29, 2024 16:25
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

Attention: Patch coverage is 85.65891% with 37 lines in your changes are missing coverage. Please review.

Project coverage is 75.23%. Comparing base (ce667ea) to head (7b8817a).

Files Patch % Lines
...creen/RoomChangePermissionsScreenCoordinator.swift 0.00% 28 Missing ⚠️
...sScreen/RoomChangePermissionsScreenViewModel.swift 88.13% 7 Missing ⚠️
...sionsScreen/View/RoomChangePermissionsScreen.swift 95.83% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           develop    #2513    +/-   ##
=========================================
  Coverage    75.23%   75.23%            
=========================================
  Files          530      535     +5     
  Lines        36848    37106   +258     
  Branches     17613    17613            
=========================================
+ Hits         27721    27918   +197     
- Misses        9127     9188    +61     
Flag Coverage Δ
unittests 60.59% <85.65%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Velin92 Velin92 left a comment

Choose a reason for hiding this comment

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

All good but I would prefer we could remove those force unwrappings, not for the risk but because they are ugly, and we can definitely get rid of them very easily

@pixlwave pixlwave requested a review from Velin92 March 1, 2024 14:22
@pixlwave
Copy link
Member Author

pixlwave commented Mar 1, 2024

Alright, I've added a defaultValue method that shares the key path so it works in both locations. WDYT @Velin92?

Copy link
Member

@Velin92 Velin92 left a comment

Choose a reason for hiding this comment

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

LGTM

@pixlwave pixlwave enabled auto-merge (squash) March 1, 2024 14:51
@pixlwave pixlwave force-pushed the doug/change-permissions branch from edeba4b to 7b8817a Compare March 1, 2024 15:30
Copy link

sonarqubecloud bot commented Mar 1, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@pixlwave pixlwave merged commit bd15190 into develop Mar 1, 2024
9 checks passed
@pixlwave pixlwave deleted the doug/change-permissions branch March 1, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants