-
Notifications
You must be signed in to change notification settings - Fork 78
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 menu option description functionality and description texts #330
Comments
This will likely build on top of the work done on Scribe/Components/InfoChildTableViewCell/InfoChildTableViewCell.swift |
Designs are all done and there's one linked up in the prototypes at this point 😊 Here's an image for documentation: |
Hey @damien-rivet 👋 @SaurabhJamadagni let me know that he's a bit backed up at the moment and can't get to this. Would you be able to support? |
Of course, I will gladly help. @SaurabhJamadagni let me know what I can do to help you contribute to the project 😉 |
Thanks @damien-rivet! Sorry I misspoke a bit :) Would you be able to pick this issue up for @SaurabhJamadagni? He mentioned that he'll be able to jump back into issues at the end of the month, but we need this for the 3.0 release that ideally would be before then 😊 |
We have the menu option |
@andrewtavis can you add me as an assignee, I'll start working on it asap. |
Thank you, @damien-rivet! Really appreciate you helping out here :) |
Hey @damien-rivet! Thank you so much for picking this issue up! I have been meaning to attend to it for a long time now but haven't been able to. Really appreciate your help! Thanks once again 😊 |
Changed static data to constants to avoid unwanted modifications. Added localization catalog to the Scribe App. Added a shortDescription field to cell model.
@andrewtavis @SaurabhJamadagni I've been looking at the necessary code to make this happen and I must say I'm not satisfied by the result at all. Expected implementationPresenting a popover from a view controller from a cell contained within another cell on the information We can either display a popover but it would require to be able to capture a reference to the We can also display an overlay above the whole screen to display the description but it would be overkill for something so "trivial" and will disrupt user's flow. Both of those solutions are sub-optimal in terms of both code quality and user experience. Suggested implementationsWhat I suggest is to stick with Apple's own HIG and own Apps (like the Settings App) that encourages us to stick with a more direct approach. The quickest implementation would be to replace or improve the custom cells to add a short description label underneath the first line (if there is something to show), it would work but would require to refactor the cells a bit. The longest implementation and the closest to Apple way of doing things would be to do things in a similar fashion as the Settings App by displaying under each section the short description if there is any. It would look like this (not captured from my phone): |
Thank you for the research and the comments here, @damien-rivet! Definitely was not expecting this level of differences between implementation for iOS and iPadOS. Your suggestion makes sense to me :) We have the small horizontal lines between the options, so we could get rid of the icons and then have the descriptive text be between the option title/chevron/toggle and the line in a bit smaller text and maybe a bit more grey? Shall I redo the designs with this in mind and send them for a check? Again thanks for this! Great to have these insights before we dig a hole :) |
Doing a quick as of yet unfinished designs, let's get rid of a lot of the menu icons all together. I think that having the icons on the about tab looks good, but we don't need the ones that are on installation and settings. Generally for a menu option I was thinking (see on Figma): I still need to design the other variants, and the vertical spacing could likely be bigger. I've done a partial redesign of the app screen in light mode so we can take a look at it for minor feedback before getting started and finishing the designs. The menu options don't have the descriptions yet, but just to show what it's looking like without the menu option icons on installation and settings :) Thanks again for the suggestions here, @damien-rivet! Should be able to finish the designs tomorrow if the start's looking like where we should go 😊 |
We could also replace the icon |
I'm not even sure we need an icon for that, the users knows that they are interacting with settings, adding a cog icon does not convey any additional information 🤔 |
Yes I agree, @damien-rivet :) Just wanted to open it up. Appreciate the feedback! I think we're good with the decision to remove the icons from the app screens except for on the About page where say the GitHub icon would convey some information to the user 😊 I'll try to finalize the designs on Figma today and also update the ones for Desktop as well! |
Ok I've just finished the update of the designs on Figma. Note that I got rid of the clickable prototype as I needed to replace most of the page elements. I can remake it if there's a need for it 😊 What's changed:
Big thing is that as things progress we need to make sure that we don't mix menu options that have a description under them with those that have a line separator within the same group as I feel this would just look off... Then again the designs reflect plans for the app for the foreseeable future, so we should be good 🙌 Thanks for the feedback on all this and the great suggestion, @damien-rivet! Let me know what you think of the changes :) |
Updated the title of the issue given the new scope :) :) |
Hey @andrewtavis, Here's the current state of the code on my side: RefactoringIn order to make the new description field show / hide and the cell to resize accordingly, I had to remove the extra layer of tableview contained in the ParentTableViewCell (the PR is gonna be nice to review 😅). I also used this occasion to generate a maximum of UI stuff using the native controls like the section header, the accessory type for the cells (the chevron in that case). There are still a few stuff I need to complete before opening a PR though:
Screenshots
|
Looking forward! 😊 Thanks for taking the initiative on this refactor, @damien-rivet :) So far it really is looking great. So happy that you've been willing to support here. I think that it makes sense that I hold off a bit on #351 until the PR is made, but I'll merge that ASAP and then work in the changes I've already made for that in relation to the new architecture. Let me know if you have any questions or comments on the designs! |
Additional clean up.
Add a base table view controller to implement similar logic between screens.
Additional clean up.
Hey @andrewtavis 👋 Would you be available to review my PR? Thanks in advance. |
I have it in mind and you have my apologies, @damien-rivet. Was in the US for the holidays and am now feeling a bit sick, but I'll try to get to it this evening :) |
Changed static data to constants to avoid unwanted modifications. Added localization catalog to the Scribe App. Added a shortDescription field to cell model.
Add a base table view controller to implement similar logic between screens.
Additional clean up.
Added separator insets to tableviews. Added disclosure indicators in About tabs to cells with nested navigation. Additional cleanups.
…tions-information-tooltips [#330] Add description texts to cells
#384 was merged and I've made a few minor issues for the final changes needed for the menu, so we can now close this 🥳 Thanks for all the work and the immense help in guiding the decisions for this, @damien-rivet! Really was so key to the project to get your help here! |
Changed static data to constants to avoid unwanted modifications. Added localization catalog to the Scribe App. Added a shortDescription field to cell model.
Add a base table view controller to implement similar logic between screens.
Additional clean up.
Added separator insets to tableviews. Added disclosure indicators in About tabs to cells with nested navigation. Additional cleanups.
Terms
Description
This issue is for menu option info tooltips - tooltip boxes that pop up with quick information on a menu option (purpose of the option, how it could change user experience, etc.) These would:
Contribution
The text was updated successfully, but these errors were encountered: