Bug improve bookmarks management #26
Conversation
|
Hum, I wonder if this is the right way to solve the issue? The ticket doesn't really explain to me what the issue is. But I think they are saying;
I don't know, but I think that this needs a design discussion. |
|
The star icon updates it color when user changes the website.( I must have forgot to demonstrate this in the above gif). Yes, we can discuss the design with the community though I tried to implement all the feature exactly as were mentioned on the feature-request page -> https://bugs.sugarlabs.org/ticket/4714 |
|
I'd de-emphasise the bug report, as it came from OLPC AU who have since abandoned Sugar deployment altogether, focusing instead on Android. The person who raised the ticket doesn't want the problem fixed any more, and didn't update the ticket to say so. I agree consensus is not yet reached on requirements, and @iamutkarshtiwari was very brave to use the ticket without checking for consensus. Good try, but let's get back to gaining that consensus; which we also call a design discussion. Use a [DESIGN] subject tag on sugar-devel@ mailing list. |
|
@quozl Shall I start a thread on sugar-devel with the [DESIGN] tag for browse activity? |
|
@iamutkarshtiwari, welcome to open source development, where your decisions are yours and sometimes people won't give advice. 😁 |
| icon = Icon(icon_name=self.icon_name) | ||
| else: | ||
| icon = Icon(icon_name=self.icon_name, xo_color=xo_color, | ||
| pixel_size=style.STANDARD_ICON_SIZE) |
There was a problem hiding this comment.
I'm amusing that this is the function that you added compared to the original toolbutton? Maybe a better approach is to:
- Send a patch in to sugar-toolkit-gtk3 with this change (it is one of those, "Didn't we have this api already?", changes)
- Have a conditional import and include a minimum viable subclass with your changes
|
So I must of missed the mailing list thread, but would it be possible to send a separate patch that does the "update star colour when the user navigates" feature? I think that it can be merged eaisly. |
|
@samdroid-apps Sorry, I did not get you? What shall I exactly do to make it easily mergeable ? And is the current proposed design acceptable ? |
|
What's the current status of this? |
|
@davelab6 There hasn't been much design discussion on this PR (though it's complete). It needs community consensus. |
|
@iamutkarshtiwari how is that consensus reached? |
|
@davelab6 We need to have a [DESIGN] discussion over mailing list to gain community consensus. But ultimately it's the maintainers' choice to merge or close. |
|
Yep, lets discus this on the ML. I personally lean towards option 2. But I think that we can merge parts of your patch, eg. the bits that change the color of the favorite icon as you navigate to a site that is favorited. |
|
Adopted option 2 in d25af13 |
This PR fixes this defect-> https://bugs.sugarlabs.org/ticket/4714
This patch implements the
Option 1feature mention on the bug^ page. AS mentioned, thestarwill nowonly togglethe bookmark tray and the pages can be added to the tray by clicking theplusbutton added to the tray. Thecolorof thestarchanges to theprofile_xo_colorif the page is added to favourites else if the page isremoved/does-not-existin the tray then color changes todefault b/w.Here is a gif to demonstrate the feature.
In case if the tray is full of a list of pages which needs scrolling, the scroll buttons can be activated by simply clicking on the
plusicon in the tray which expands to show theleft/rightscroll icons and the page can be then added by again clicking over theplusbutton.Here is a gif to demonstrate this feature.