-
Notifications
You must be signed in to change notification settings - Fork 117
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
Epic: create and join rooms - Start Chat #680
Conversation
Generated by 🚫 Danger Swift against 0e3e2a6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this is really great first PR to the project 🙌 I see you found all of the form style modifiers. Hope they were self explanatory enough to use.
I'm sure you've noticed Unit tests need implementing, its probably worth pointing out UI tests are the same. Those run nightly so its down to us to remember to check them locally first. Various other comments inline.
One of our aims with the Element X project is to avoid lots of warnings popping up in the sidebar. Ideally if we could avoid merging lots of // TODO:
comments this would be better for everyone. I was chatting to Stefan about maybe doing something like // ISSUE:123
instead so that they aren't warnings, but easily searchable and have an associated issue so everyone knows what is needed. Might bring this up on Friday for everyone to chat about :)
ElementX/Sources/Screens/DeveloperOptionsScreen/DeveloperOptionsScreenModels.swift
Outdated
Show resolved
Hide resolved
ElementX/Sources/Screens/DeveloperOptionsScreen/View/DeveloperOptionsScreenScreen.swift
Outdated
Show resolved
Hide resolved
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #680 +/- ##
===========================================
- Coverage 52.61% 51.93% -0.68%
===========================================
Files 265 270 +5
Lines 14454 14647 +193
Branches 9150 9246 +96
===========================================
+ Hits 7605 7607 +2
- Misses 6623 6814 +191
Partials 226 226
Flags with carried forward coverage won't be shown. Click here to find out more.
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 in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🎉
Co-authored-by: Alfonso Grillo <alfogrillo@element.io>
…ment-x-ios into epic/create_and_join_rooms
Super, screenshots look 👌. The unit tests are failing due to not having run SwiftFormat. I think you need to run the project setup script to get this to run on commit.
Then in comparison to EI we have more checks to look at. So please look at fixing the SwiftLint warnings and SonarCloud code smells that would be awesome. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 😎
Close #592
Add the button for starting the Star Chat flow, the button present the screen with only UI, more login in different tasks