Favourite tab added in the sidebar #6553
Conversation
✅ Deploy Preview for plone-components canceled.
|
|
All checks have passed successfully and the code is ready for review. I've completed the requested changes. Requesting a review and merge from someone with write access since I don't have merge permissions. Thanks! |
|
@TechSubham is this a feature enhancement per #2039? If so, I think @tisto would be interested in this feature, and you should amend the Description of the PR with " |
There was a problem hiding this comment.
The default language is en-US. Please change the spelling of all instances of "Favourite" to "Favorite".
Also remove *.crdownload files.
Also we have icons available that you should use in Pastanaga, instead of adding new ones. See https://6.docs.plone.org/volto/contributing/icons.html for usage.
|
I’ve made the requested changes to the branch: Updated all instances of "Favourite" to "Favorite" to align with en-US language standards. Please let me know if there are any further adjustments or if this branch is ready for review. |
|
@TechSubham I think you made a mistake, as you did not push the requested changes to this branch. See https://6.docs.plone.org/contributing/first-time.html#update-your-pull-request-from-your-fork for how to resolve the issue. |
TechSubham
force-pushed
the
Favourite-Tab
branch
3 times, most recently
from
c4f655c to
2df32cc
Compare
TechSubham
force-pushed
the
Favourite-Tab
branch
from
2df32cc to
de35862
Compare
There was a problem hiding this comment.
This is much better. Thank you!
I think this PR also needs documentation in the User Manual as a new section in https://6.docs.plone.org/volto/user-manual/copy-paste-blocks.html, since favorites are a kind of copy/cut and paste operation. Do you agree?
This is a good exercise for first-timers. Code, tests, and documentation. Keep up the good work!
|
i think there's too much buttons on the right now, and it's not the better solution for mobiie use.. isn't it better to move 'save button' on the top, or bottom of the block like the 'add button'? |
packages/volto/src/components/manage/Sidebar/__snapshots__/Sidebar.test.jsx.snap
Show resolved
Hide resolved
| @@ -0,0 +1,7 @@ | |||
| .absolute-div { | |||
There was a problem hiding this comment.
please make this rule more specific, like .drag.block .absolute-div
| @@ -1237,6 +1237,16 @@ div.image-upload-widget-image { | |||
| } | |||
| } | |||
|
|
|||
| .parent-class { | |||
| .absolute-div { | |||
There was a problem hiding this comment.
why here another rule with same styles definide in save.css?
| className="delete-button" | ||
| aria-label={intl.formatMessage(messages.delete)} | ||
| style={{ | ||
| marginRight: '3px', |
There was a problem hiding this comment.
move styles into less file
| > | ||
| <Icon name={trashSVG} size="18px" /> | ||
| </Button> | ||
| <div class="absolute-div"> |
There was a problem hiding this comment.
i think it's better to change class name with a more specific name for this case
| > | ||
| <Icon name={trashSVG} size="18px" /> | ||
| </Button> | ||
| <div class="absolute-div"> |
There was a problem hiding this comment.
| <div class="absolute-div"> | |
| <div className="absolute-div"> |
| onClick={() => setIsSaveDialogOpen(true)} | ||
| className="save-button" | ||
| aria-label={intl.formatMessage(messages.save)} | ||
| style={{ |
There was a problem hiding this comment.
move styles into less file
| @@ -6,7 +6,6 @@ import Icon from '@plone/volto/components/theme/Icon/Icon'; | |||
| import { flattenToAppURL } from '@plone/volto/helpers/Url/Url'; | |||
| import { getContentIcon } from '@plone/volto/helpers/Content/Content'; | |||
| import config from '@plone/volto/registry'; | |||
|
|
|||
There was a problem hiding this comment.
re-add this row to unchange this file
| type="button" | ||
| className="sidebar-copy-button" | ||
| style={{ | ||
| position: 'absolute', |
There was a problem hiding this comment.
move styles into less please
| @@ -1237,6 +1237,16 @@ div.image-upload-widget-image { | |||
| } | |||
| } | |||
|
|
|||
| .parent-class { | |||
There was a problem hiding this comment.
what is .parent-class ?
| import trashSVG from '@plone/volto/icons/delete.svg'; | ||
| import SaveBlockDialog from '@plone/volto/components/manage/Blocks/Block/SaveBlockDialog'; | ||
| import './save.css'; |
There was a problem hiding this comment.
please, create a less file under theme/pastanaga
| .sidebar-container { | ||
| } | ||
| // .sidebar-container { | ||
| // } |
There was a problem hiding this comment.
Instead of commenting code, remove it. It's just noise as a comment.
IMO, I think the Favorites belong under the "Block" tab in the upper-right. After all, we are working with favorite blocks. I don't like how I have to click the paste icon, then mouse over from the upper-right to the lower-left to paste a favorite block. Why not just single-click and paste the block? Also it is strange that I have to click on the name of the block to open the Copy Block dialog. I'd prefer a copy icon/button. |
|
@stevepiercy my PLIP is about the content tree's locations, not favorite blocks. The feature itself is certainly useful. However, I doubt we can integrate this feature into the user interface without completely overwhelming the editors. Maybe providing this feature as an add-on and gaining more experience with it would be a way to go. |
|
@tisto are you sure that's the right issue? It's not labeled as a PLIP, and its title and description is for adding this exact favorites feature. |
|
@stevepiercy yes. I am sure about this. I recall creating this ticket after a client request that we got. So far, I never heard the request for favorite blocks (not saying the feature isn't useful). My thoughts about the UX:
To sum up. I like the feature. However, solving this UX-wise is non-trivial. |
|
@tisto this is the link to what you called "my PLIP": That is not a PLIP. It is a feature request for favorites. What is the link to the PLIP you are talking about? |
@stevepiercy I wonder if @tisto didn't mean to say the content browser, imagine adding an image block and using the object browser where you want to select images and you know it's always from /assets folder. You would have a favorite to that folder and make it easier to get to that path rather than navigating around until you find the asset folder from your current location. @tisto is my interpretation valid or am I wrong as well? |
|
@ichim-david you are correct. Sorry for writing a crappy enhancement proposal and not referring to it correctly here. @stevepiercy yes. #2039 is "my PLIP". |
@stevepiercy @tisto @TechSubham and here we have a prime example of being safer to ask first if a request is still valid and if it is to run by the issue author if what you think needs to be done aligns with what the author thought. This is a pretty interesting feature and it might make sense to have as an add-on indeed. If it gains some tracks we could think of how to integrate it into Volto core. @TechSubham as a new contributor to Volto it's always best to ask first before starting to implement something as that gives the best chance for your work not to go to waste on account of different priorities or visions for a feature of bugfix. I don't know if you will have any desire to turn this code into an add-on or not and if you will receive any UX feedback further on this feature. I am not dismissing this work nor am I +1 or -1 against it, I'm simply trying to give you some advice on how to approach future contributions to avoid any disappointment. |
|
@eveyone Thank you for taking the time to provide detailed feedback and guidance. I genuinely appreciate the opportunity to collaborate with such experienced contributors, and your insights are incredibly valuable to me as a new contributor to Volto i will make sure to ask the details about the issue from the author from next time onwards |
|
I really want to test this feature, as soon as I have time, I’ll take a look. |
@tisto
Agreed they should be in the chooser. I don't think they should be in the sidebar at all. if you want to manage them it should perhaps be in the site settings or personal preferences. I have a plan I will outline later which involves combining the idea of shared saved blocks like this, volto layouts and read only blocks, all into one concept. Because generally if you are reusing content/blocks it's more likely done at an origanisational level and there is a good chance you want the ability to change reused content in multiple places rather than copying it. This is all a hard thing to get right which I why I think this feature could be premature.
@ichim-david I couldn't agree more and I'm suprised to hear you say this given you have seemed to think in the past it's always better to implement a UI first. I personally think there should be a seperate design forum for plone called "design advice needed" where contributors/developers can get advice on the best UX for new features, plugins or their own client work. Here is not the best place. This might help us get to more well thought out UX, with less wasted effort |
|
@TechSubham Thanks for the effort on putting together this implementation. We all know how hard it is and the time you spend on it. Also, thanks for approaching Plone, and thinking that you could improve it! As other members of the community already mentioned, Plone has a process to decide what gets in Plone and what does not: The PLIP process The Plone codebase, and the people that uses it is huge and we take very seriously the process to ensure consistency and expectations. I encourage you to continue the process, given this proof of concept that you provided. @plone/volto-team Let's put this in the pipeline. Certainly, the "saved blocks" feature is a nice to have one, but it has to be rationalized and discussed, same for the resultant UX. Honestly, this stress out the feeling that I have that the current "CMSUI real state" in screen has reached its limit, and we need to start thinking in other solutions. However, this is part of a greater and complex discussion. |
@djay My message has always been from the start to check with the authors of any feature request or bug fix if that request is still valid and to have a short comment where you present what you think would be necessary in order to fix it or have a screenshot with what you think would be fine as a feature. Even more so for first-time contributors who are eager to jump on the occasion to add a pull request without knowing the entire context that is gained only after spending a considerable amount of time in the community and using Volto. In our past conversations, I have stressed about not letting the quest for perfection bog down the good enough enhancement. I strongly believe that this is the case to this day because I would much rather have three quick fixes that are released in a timely fashion rather than one major fix that is released after six months which means that for six months users will not benefit for any of the fixes that could have been added had we not strived so much for perfection. That can mean UI changes or non-user visible changes, it makes no difference as long as it's an improvement. There is a case where you need to have a long process of thinking implementation and testing such as when we have some strategic flips that majorly impact our users in the way Volto looks and behaves like. For situations like those it makes sense to have a process where you think it through and you try to make it right. Our time is limited. Our funds are limited. Our manpower is limited. We don't have the luxury to spend so much time when we know that time means money. If you spend a lot of time, that time could be spent working and getting paid for that. If you would get paid to contribute I'm sure that any company that would be willing to pay to develop something and open source it would not appreciate it if the costs skyrocketed due to long reviews and change requests or to see the effort be stuck and never making it on the product. If you are spending so much time going over discussions over discussions or having a pull request get stuck in limbo because of some nit or some extra step that could be added or fixed in another pull request, you really decrease the joy, the desire, and the effectiveness of the person who would be willing to put in the time to simply make this open source software 0.1% better. Not everyone has the time, the resources and the ability to make this project 10, 20, 50% better because they introduce a major feature or major bug fix. Even when that feature is created, it would be so large that it would take a massive amount of effort to review especially if it has a large architectural change, that it would be very hard for an individual or a company or the community to simply take the time to review it in a timely fashion. As such, yes, I would much rather have small pull requests with tiny fixes that if there is a problem, it's very easy to review and very easy to revert and if everything works out, then again, you have something that's a bit better than it was before you touched it. It would also be quite a great momentum builder or booster for both to the individual who gets his pull requests merged and to the reviewer who is able to quickly review and merge a pull request that makes something better to this software that we use and we all want to see it improve. Everyone is entitled to use their time in any way they see fit. I know you are a person who is deeply dedicated to improving the user experience aka the UX and the UI of Volto and many times that requires a significant investment both in time and money and also vision. Personally, I don't have the energy and the time to engage in such projects. However, due to my love for this community and the software that I use daily for my work, I would love to see pull requests from individuals who are focused and simply trying to make one fix or improvement to Volto. Everyone expects features and things to improve and be visible from month to month. If you have nothing to show for it as time has passed, you will not get any support going forward because people will lose trust in your ability to improve the product or show that you are actively working on it. So yes, I want to show that I am working on something, and even though I am not contributing to the greatest feature or bug fix, the steady improvements will be appreciated especially by the users who use the software daily and run into some of the issues that they would love to have at least partially fixed. |
We also carefully documented this process, especially for first-timers, in Work with GitHub issues. |
@stevepiercy and this is a testament to your huge efforts to improve our documentation, our processes to deal with and think of the first-time contributors. This work was not done in one go, it took several iterations and it will improve again in the future as we come out with ways to improve our work. I really appreciate all that you are doing for Volto and all of the work on checking every pull request. These improvements would not be possible with only mammoth pull requests from 1 user that would just lead to burnout. @djay Our docs are not perfect by any imagination, but this iterative approach allowed @erral to write some docs, @stevepiercy to proofread and me to review which meant that now we have 3 extra examples for blocks as seen in this pull request #6560. I've told @erral that it would have been nice to have an example using an Edit view but I meant when I said that it was not mandatory to get his work merged given the fact that it already contained 2 new examples that we didn't have documented up to this point. So taking in my own dogfood advice, I wouldn't let the perfect get in the way of the iterative approach to improve Volto in any facet. |
|
@ichim-david UX is different. Add one thing here and it can make things more confusing over there or harder to do. |
Screen.Recording.2024-12-23.at.2.20.05.AM.mov