-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[MBL-958] Ai page polish #1847
[MBL-958] Ai page polish #1847
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1847 +/- ##
==========================================
- Coverage 84.61% 84.60% -0.02%
==========================================
Files 1279 1276 -3
Lines 116126 116026 -100
Branches 30921 30892 -29
==========================================
- Hits 98258 98158 -100
Misses 16786 16786
Partials 1082 1082
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Yea, good simplification for this cell and nice styling for the green bar.
I see what you were saying about the separator under "Learn more", but it's fine to let go. All the other separators on this page are fine.
@@ -64,6 +63,11 @@ public struct ProjectTabCategoryDescription { | |||
public var id: Int | |||
} | |||
|
|||
public struct ProjectTabGenerationDisclosure: Equatable { |
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.
This is a better organized strut - did you consider what happens if there are more than two string sections in the future?
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.
I'm not sure I understand the question. Do you mean what we'd do if the backend wants to add another question and answer to this section? Because if so, I think we'd either just create another view like the two we have, or we could use enums with an associated string (enum value representing the question and associated value representing the user-provided string) to simplify the code a bit. It'd be similar to the way we currently handle the checkmark cell. Though hopefully by that point, this will be rewritten in swiftui!
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.
lol, sure. I was just thinking we'd just send an array of strings into this function and in the cell, do a forEach
on each String
to create the views.
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.
We'd still need a way to tell what question/title each string is associated with, so we can't just do an array. But let's shelve this for if it becomes relevant :)
📲 What
Convert the current "AI generation section" to all be one cell and update formatting to match android/web.
🤔 Why
We want to show separators between the different AI sections and green bars at the leading edge of the answers. This wasn't done when the questions were first added since we didn't have time to do so and get the code in the cut.
🛠 How
I considered both this approach and the approach of keeping the ai generation section as three different cells. I ultimately went with this approach because it felt more internally consistent to make the title part of the cell (and if we wanted to move the title out of the cell for all the titles on the AI page, we'd need to modify the environmental commitments tab as well) and because this felt less hacky than adding a
showSeparator
orendOfSection
var
to the current question answer cell. Happy to revisit this if anyone is curious; I mostly coded up both approaches while deciding what to do.👀 See
JIRA
✅ Acceptance criteria