-
Notifications
You must be signed in to change notification settings - Fork 93
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
Implement 'Show more / Show less' button to longer texts on About page #1091
Comments
I am starting to work on it =] |
@AndreyGoranov @ivoka9 We've got an interesting situation regarding this task. We've ended up having two separate PRs with different implementations of the same reported problem. Now we've got to keep only one of the following two versions which solves the problem. I invite you to collaborate and help the team decide how to proceed. I see two authentic ways to solve the task, now we've got to decide if we keep In order to avoid this double work again I urge everyone to check if the ticket has an assignee and pick another available one instead. |
First of all I want to start with an apology for not assigning it to myself, but for the life of me I could not figure out how to as I don't see the button, (based on my google search I have to first contribute in order to be able to assign myself ?) Second I do find it a bit hard to criticize my own method as I am biased, but here is my takeaway My method is more simple, and maybe under some conditions will break as I use hardcoded values 50/200 etc... His method seems more advanced as it gives a certain number of rows that would display, also it takes the user screen view into consideration, making some "complex equations" to consider what should show. I am more then happy to collaborate on this or any other issue but assuming that he is Bulgaria I am currently in the US there is 7 hours difference it it would make it a nightmare. If possible close it with the best possible solution or take my feedback and give me/him the ability to finish what we have started, and lets move to the next complicated ticket :) |
I personally prefer solution B because the implementation seems simpler and it would be easier to maintain. |
I guess my approach might be an overkill for the app needs. My first approach was simply passing a string limit and using the substring method. But I didn't like the UX so I tried making it more smooth. About the "Покажи повече" it can always be changed to a button. If we plan using more expandable texts around the app this could serve well but I am still getting to know the app so I don't really know whats best at that point. |
@AndreyGoranov, @ivoka9 Your involvement means a lot! Thank you! Since we have the two solutions for about a week and need to merge one of them, can we agree to merge solution B having in mind the communication above? Please advise! As @kachar mentioned please assign yourself the tickets which you are working on in order to avoid such situations. |
@ani-kalpachka I am ok with it. |
Steps to reproduce:
Expectation: Implement 'Show more / Show less' buttons under the longer descriptions.
The text was updated successfully, but these errors were encountered: