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
181 responsive UI #1615
base: master
Are you sure you want to change the base?
181 responsive UI #1615
Conversation
…appropriate places when the UI is solid )
… an if statement checking the breakpoint
…ly revert some things once the mobile UI is solid, then patch up desktop view to its original state
…t more space for mobile usability
…ate places, remove redundant css, update template to work with mobile UI )
…ckbox on mobile UI
…s, start work on operation/category lists
…the appropriate moments
… file, add new css files to split up huge IO stylesheet for better future DevX
…ve overal structure in layout/_structure
…ate, so it is not visible anymore during load. Update TODO.md accordingly
…entirely sure what the issue was, but something in that checkbox decorator code caused reordering / cloning of c-recipe-li to fail
…e the existing UI tests
Hey! Continuing from Discord Can you please add the functionality of these 2 items to make it more accessible? :) #1360 and this feature request #1409 Also on mobile if you scroll with your finger (down past the page, you cant do this on desktop but you can force scroll on mobile to the white at the bottom) some text follows you, like this text: very minor bug for people who are forcibly scrolling through websites :) |
Hey @valdelaseras, apologies for taking so long to look at this truly behemoth PR. An initial comment when testing this is that it seems to break on roughly half the operations. For instance, trying to add 'entropy' creates the following error:
After this error displays, the panels are no longer draggable and no input parameters are rendered for the component. This error might have gone unseen because you can double click operations to add them still. The problem doesn't appear if you refresh the page whilst maintaining these inputs, or by entering in a new deeplink. So it seems to be something to do with the instantiation after dragging? Some other minor things:
As an extension that I wouldn't see as necessary to the PR:
Initially, I was also going to suggest that the 'operation' menu should close after every new operator added, but in retrospect I could imagine that getting frustrating. Once again, thanks for the incredible amount of work you clearly put into this. It'll likely take us several more weeks to run through all your changes. |
Hi @a3957273, thank you for your comments and observations. I understand it takes time to work through the entire PR, no worries. Please take your time and collect any feedback you have and issues you find, then I will be sure to make time to immerse / dedicate myself into this codebase again and get this PR ready. I'm also planning to tackle the two additional requests and the bug @bee-san mentioned above at that time. The UI remarks you mentioned seem easy enough to update. I've also considered a counter for 'duplicate' Operations, but was a little unsure if it was necessary, as you have to remove any individual Operations from the Recipe manually (the user would see how many duplicates there are). However, I agree that it would be better if there was some way to see that from the dropdown itself and to have visual feedback of the Operation being added as a duplicate. I don't think it will be very hard to add a little counter next to the checkmark. I do not recall the error but I'm confident I'll be able to fix it. Thank you for reporting that. I'm looking forward to your further feedback! Cheers |
…e searchbox border nicely
… 40), as per the current width implementations on init
Wow, this PR is really fantastic! Whilst I'm cautious about the "more lines of code equals less comments" meme, there really are very few comments I have on improving this. You're absolutely correct in your first prompt that a lot of these changes are actually just moving things around, and most of the rest are adopting native web components. Both these changes would be fantastic alone, let alone that this introduces a far superior mobile UI. In general, if you manage to solve the:
Error that occurs with some operations I'd be happy to get this merged in. 👍 Thanks once again for the work you've put into this. |
Thank you a3957273, I'll make time soon :) |
Took us long enough to review it, so no hurries. Just whenever you get around to updating it! |
@a3957273 sorry, could you please describe how you're getting the error? I can't seem to reproduce it, operations are added normally which makes it a little tricky to debug this. Perhaps it's the drag event specifically or something like that. Any additional info would be super helpful :) |
This belongs to #181 Misc: Mobile UI
Heya CC,
This was quite an adventure :) I am well aware that this is a huge PR, so I will try to give a handy overview to help with your decision making and understanding why and what. Please do not be intimidated by the delta on first sight, a lot of these changes are ( essentially ) the same code, but moved to other places.
Changes
Code
Native web components
The most significant change. After a lot of trial and error trying to make the mobile UI work with minimal changes to the existing codebase, ultimately I decided I needed to do more rigorous refactoring to decouple and simplify the code. A big effort, but the right way to go in my opinion.
Please note that I have refactored only the code necessary to make the responsive UI work. That is something I think is important to be mindful of: there would have to be more work to turn everything into web components consistently.
src/web/components/
prefixed withc-
are replacements of the current HTMLOperation, HTMLCategory files plus a lot of functionality gathered from a variety ofwaiters
,App
andmanager
that could be moved to the specific components.CSS
UI tests
UI changes
I have made a few UI changes that do not only apply to the mobile UI. I left some mobile features in the desktop UI as well to reduce code complexity, and I think in general these changes are good for the overall user experience anyway.
The desktop UI,
operations
andrecipe
, will look like below.io
remains the same.Favourites
favourites
favourites
remains the samefavourites
remains the sameSelected
recipe
, have a checkmark iconrecipe
is still possible and the checkmark will stay visible until all of those duplicate operations are removed fromrecipe
The mobile and tablet UI looks like the following:
A new addition: maximised
recipe
pane for a comfortable user experience ( mobile / tablet only ):Pane maximisation
recipe
,input
andoutput
panes. Mobile screens are small and finnicky and a lot of user interaction is required to use CyberChef. This way, users can have a more comfortable experience configuring their recipe, reordering, inspecting output etc.Favourites
Favourites
by tapping the star icon. This was necessary as you can't add favourites by dragging when theoperations
list itself is scrollable, that would be a very annoying experience.Selected
categories
are not visible side by side withrecipe
on mobile like they are on desktop, the user needs some type of feedback to know if an operation was added torecipe
. Hence, the selected icon as mentioned aboveTabs
input
( and consequentlyoutput
) component is disabled. It's too finicky on mobile in my opinion.I've been working on this intermittently since April, so I hope I have not forgotten to mention any major change above. Feel free to ask if you have any questions or remarks. Take your time to test the UI yourself, I have added some configuration that allows you to easily test on your actual mobile device when you run
npm start
. I have put a lot of time into thinking of an intuitive UX for mobile, so I hope you will be able to use it easily on a mobile device :)Cheers!