GNOME Bugzilla – Bug 759922
Add tooltips to button that are missing
Last modified: 2018-01-19 12:37:35 UTC
Are there buttons in Maps that would benefit from having tooltips? From an accessibility point of view? A review of this and code to add tooltips would be appreciated! One can check the code in main-window.ui to see examples of tooltips added for headerbar buttons.
The zoom in or out buttons could do with some tool tips. Also the search bar could have some default text such as "Search for places.." or something like that. Apart from that I don't think other tool tool tips can be incorporated. Anyone else? Suggestions?
The route planner slider bar could also do with some tool tips. The destination and the departure place for example. The icons for driving,walking and cycling makes a lot of sense already.
I am new to gnome-maps. I think the route planner sidebar should have some tooltips on add button and the remove button. the tooltips could be "Add a via point" and "Remove the via point". And may be some way to let user know that he can drag and drop the entries by dragging and dropping the icon.
Created attachment 322409 [details] [review] sidebar-tooltip: added tooltip to add and remove via point Added an "Add a via point" tooltip and a "Remove the via point" to improve accessibility.
Review of attachment 322409 [details] [review]: ::: src/routeEntry.js @@ +69,3 @@ this._buttonImage.icon_name = 'list-add-symbolic'; this._icon.icon_name = 'maps-point-start-symbolic'; + this._button.set_tooltip_text("Add a via point"); I was thinking about if these labels would be a bit technical. Maybe something like "Add location to visit in route"?
I think we would possibly need some designer input on the labels?
Also, this will need a string announcement and UI freeze exception.
Created attachment 322458 [details] [review] [Patch]sidebar-tooltip: added the tooltips to sidebar button added tooltip "Add location to visit in route" to button to add via point and "Remove the location from route" to remove the via point so that it makes more sense to users
Review of attachment 322458 [details] [review]: ::: src/routeEntry.js @@ +69,3 @@ this._buttonImage.icon_name = 'list-add-symbolic'; this._icon.icon_name = 'maps-point-start-symbolic'; + this._button.set_tooltip_text("Add location to visit in route"); These strings should be marked for translation. Like _("Add location to visit in route")
Created attachment 323105 [details] [review] sidebar-tooltip: added the tooltips to sidebar button added tooltip "Add location to visit in route" to button to add via point and "Remove the location from route" to remove the via point so that it makes more sense to users
Created attachment 323106 [details] [review] Sidebar-tooltip: added the tooltips to sidebar button Added tooltip "Add location to visit in route" to button to add via point and "Remove the location from route" to remove the via point so that it makes more sense to users
Review of attachment 323106 [details] [review]: LGTM
Created attachment 323227 [details] [review] MapType Tooltip: Added tooltips to MapTypes Added tooltips "Street View" and "Aerial View" to the two Map Type buttons, streetLayerButton and aerialLayerButton respectively in the layers-popover.ui
Sorry for my absence from this bugzilla entry. I want to make sure we get the 3.20 release out before returning to this. We are in string freeze. That means no new strings added, so that translators have time to work. I will pick this up when we branch master for 3.22. Thanks!
Thanks Kaustav for the patch! I also was thinking about if we should use "title case" or not for these strings. I.e. "Street View" vs. "Street view". I think it varies a bit in the menus, so perhaps, after master is open for new strings, go through this and try to normalize it.
@ Jonas : oh ok sure . @ Marcus : Alright , I will normalize it .
I was going through the strings in the menu , i noticed that the other menu options were not in title case but in the layer popover , the Load Map Layer button is in title case , so what do you say ? should i normalize the street/aerial view or keep it as it is to match the Load Map Layer button ?
*** Bug 788951 has been marked as a duplicate of this bug. ***
Not only tooltips are needed but also some labels. GtkLabel. Maybe placeholder text.
I am new to gnome-maps. I would like to work on this. Adding tool tips or labels to various buttons and text area especially to the route planner sidebar will improve usability. I would also like to propose a feature of allowing the user location as the route planner starting point by default.
(In reply to Vibhanshu Vaibhav from comment #20) > I am new to gnome-maps. > > I would like to work on this. Adding tool tips or labels to various buttons > and text area especially to the route planner sidebar will improve usability. > > I would also like to propose a feature of allowing the user location as the > route planner starting point by default. Hi Vibhanshu! Thanks for taking interest in Maps! I think adding tooltips to some of the buttons in the routing sidebar sounds like a good idea. Like the buttons to add/remove destinations and reversing a route. These buttons are generated dynamically and not part of a .ui file. You can take a look in src/routeEntry.js where these buttons are created (in a switch-statement). Did you try building and running Maps (i.e. using Flatpak from gnome-builder)? Having the user location as a default starting point might be interesting. I think adding this might need some UI design (i.e. what happens if you enter a starting point manually, and then erase it, should it be a way to reset to the current location?). In a way something similar is possible today, if you open a place "normally" using the "global" search and in the place bubble click on the route button, it will create a route from your current location to that place.
(In reply to Marcus Lundblad from comment #21) > (In reply to Vibhanshu Vaibhav from comment #20) > > I am new to gnome-maps. > > > > I would like to work on this. Adding tool tips or labels to various buttons > > and text area especially to the route planner sidebar will improve usability. > > > > I would also like to propose a feature of allowing the user location as the > > route planner starting point by default. > > Hi Vibhanshu! > Thanks for taking interest in Maps! I think adding tooltips to some of the > buttons in the routing sidebar sounds like a good idea. Like the buttons to > add/remove destinations and reversing a route. These buttons are generated > dynamically and not part of a .ui file. You can take a look in > src/routeEntry.js where these buttons are created (in a switch-statement). > Did you try building and running Maps (i.e. using Flatpak from > gnome-builder)? > Having the user location as a default starting point might be interesting. I > think adding this might need some UI design (i.e. what happens if you enter > a starting point manually, and then erase it, should it be a way to reset to > the current location?). In a way something similar is possible today, if you > open a place "normally" using the "global" search and in the place bubble > click on the route button, it will create a route from your current location > to that place. Yes I've successfully built and run Maps on my Linux(Ubuntu 16.04) PC with Flatpak. Clicking on the route button on the place bubble of a location when searched globally does works the way you mentioned. But when you simply open the route planner sidebar, all the entries are empty. From a user's perspective they would usually look for directions from their current location itself so maybe if starting point entry in the route planner is automatically set to the current location the user would just have to enter the destination. They can also change the starting point if they wish to by just deleting and entering a new location.
Created attachment 366949 [details] [review] Tooltips to Route Planner Buttons Added tooltips `Add in-route location`, `Remove in-route location` and `Reverse Route` to the Route Planner Sidebar Buttons for improved usability.
Review of attachment 366949 [details] [review]: We try to follow the GNOME commit message guidelines. The commit message should start with the module name (without the extension .js), the actual file that is changed is appearant from the commit itself. Also there shall not be title case in this string. So something like: routeEntry: Add tooltips for the route planner buttons ::: src/routeEntry.js @@ +69,3 @@ this._buttonImage.icon_name = 'list-add-symbolic'; this._icon.icon_name = 'maps-point-start-symbolic'; + this._button.set_tooltip_text("Add in-route location"); You could set these using the property directly instead of calling the setter, also the string should be marked as translatable, and along with that I think there should be a comment as a hint for translators that this is a tooltip text (the comment will appear in the translated template files that translators fill in). Also "in-route" sounds a bit weird to me, maybe something like "Add via location" would sound better? So something like: /* Translators: this is a tooltip */ this._button.tooltip_text = _("Add via location"); You will also need to define the _ as the gettext function at the top of this file with: const _ = imports.gettext.gettext; @@ +74,3 @@ this._buttonImage.icon_name = 'list-remove-symbolic'; this._icon.icon_name = 'maps-point-end-symbolic'; + this._button.set_tooltip_text("Remove in-route location"); Like above… @@ +79,3 @@ this._buttonImage.icon_name = 'route-reverse-symbolic'; this._icon.icon_name = 'maps-point-end-symbolic'; + this._button.set_tooltip_text("Reverse Route"); I think we should probably not use title case for tooltip texts, so "Reverse route"
(In reply to Marcus Lundblad from comment #24) > Review of attachment 366949 [details] [review] [review]: > > We try to follow the GNOME commit message guidelines. The commit message > should start with the module name (without the extension .js), the actual > file that is changed is appearant from the commit itself. Also there shall > not be title case in this string. > So something like: > routeEntry: Add tooltips for the route planner buttons > > ::: src/routeEntry.js > @@ +69,3 @@ > this._buttonImage.icon_name = 'list-add-symbolic'; > this._icon.icon_name = 'maps-point-start-symbolic'; > + this._button.set_tooltip_text("Add in-route location"); > > You could set these using the property directly instead of calling the > setter, also the string should be marked as translatable, and along with > that I think there should be a comment as a hint for translators that this > is a tooltip text (the comment will appear in the translated template files > that translators fill in). Also "in-route" sounds a bit weird to me, maybe > something like "Add via location" would sound better? > So something like: > /* Translators: this is a tooltip */ > this._button.tooltip_text = _("Add via location"); > > You will also need to define the _ as the gettext function at the top of > this file with: > const _ = imports.gettext.gettext; > > @@ +74,3 @@ > this._buttonImage.icon_name = 'list-remove-symbolic'; > this._icon.icon_name = 'maps-point-end-symbolic'; > + this._button.set_tooltip_text("Remove in-route location"); > > Like above… > > @@ +79,3 @@ > this._buttonImage.icon_name = 'route-reverse-symbolic'; > this._icon.icon_name = 'maps-point-end-symbolic'; > + this._button.set_tooltip_text("Reverse Route"); > > I think we should probably not use title case for tooltip texts, so "Reverse > route" Thanks for helping me out with this. I've made the changes to the code and I'll follow the proper commit guidelines in my next patch. But I'm stuck with how to mark the string translatable. Also, can you please guide me to some good documentation so that I can understand the code on my own as it would help me with solving other bugs.
(In reply to Vibhanshu Vaibhav from comment #25) > (In reply to Marcus Lundblad from comment #24) > > Review of attachment 366949 [details] [review] [review] [review]: > > > > We try to follow the GNOME commit message guidelines. The commit message > > should start with the module name (without the extension .js), the actual > > file that is changed is appearant from the commit itself. Also there shall > > not be title case in this string. > > So something like: > > routeEntry: Add tooltips for the route planner buttons > > > > ::: src/routeEntry.js > > @@ +69,3 @@ > > this._buttonImage.icon_name = 'list-add-symbolic'; > > this._icon.icon_name = 'maps-point-start-symbolic'; > > + this._button.set_tooltip_text("Add in-route location"); > > > > You could set these using the property directly instead of calling the > > setter, also the string should be marked as translatable, and along with > > that I think there should be a comment as a hint for translators that this > > is a tooltip text (the comment will appear in the translated template files > > that translators fill in). Also "in-route" sounds a bit weird to me, maybe > > something like "Add via location" would sound better? > > So something like: > > /* Translators: this is a tooltip */ > > this._button.tooltip_text = _("Add via location"); > > > > You will also need to define the _ as the gettext function at the top of > > this file with: > > const _ = imports.gettext.gettext; > > > > @@ +74,3 @@ > > this._buttonImage.icon_name = 'list-remove-symbolic'; > > this._icon.icon_name = 'maps-point-end-symbolic'; > > + this._button.set_tooltip_text("Remove in-route location"); > > > > Like above… > > > > @@ +79,3 @@ > > this._buttonImage.icon_name = 'route-reverse-symbolic'; > > this._icon.icon_name = 'maps-point-end-symbolic'; > > + this._button.set_tooltip_text("Reverse Route"); > > > > I think we should probably not use title case for tooltip texts, so "Reverse > > route" > > Thanks for helping me out with this. I've made the changes to the code and > I'll follow the proper commit guidelines in my next patch. But I'm stuck > with how to mark the string translatable. > I forgot to mention that this file also needs to be added to the list in po/POTFILES.in You could look in src/transitLegRow.js for an example of how to programmatically set tooltip texts on buttons (around line 108). > Also, can you please guide me to some good documentation so that I can > understand the code on my own as it would help me with solving other bugs. We don't currenly have any good overall architectural overview of the source (we probably should, though). But at least some is at https://wiki.gnome.org/Apps/Maps/Resources, such as links to GJS docs.
(In reply to Marcus Lundblad from comment #26) > (In reply to Vibhanshu Vaibhav from comment #25) > > (In reply to Marcus Lundblad from comment #24) > > > Review of attachment 366949 [details] [review] [review] [review] [review]: > > > > > > We try to follow the GNOME commit message guidelines. The commit message > > > should start with the module name (without the extension .js), the actual > > > file that is changed is appearant from the commit itself. Also there shall > > > not be title case in this string. > > > So something like: > > > routeEntry: Add tooltips for the route planner buttons > > > > > > ::: src/routeEntry.js > > > @@ +69,3 @@ > > > this._buttonImage.icon_name = 'list-add-symbolic'; > > > this._icon.icon_name = 'maps-point-start-symbolic'; > > > + this._button.set_tooltip_text("Add in-route location"); > > > > > > You could set these using the property directly instead of calling the > > > setter, also the string should be marked as translatable, and along with > > > that I think there should be a comment as a hint for translators that this > > > is a tooltip text (the comment will appear in the translated template files > > > that translators fill in). Also "in-route" sounds a bit weird to me, maybe > > > something like "Add via location" would sound better? > > > So something like: > > > /* Translators: this is a tooltip */ > > > this._button.tooltip_text = _("Add via location"); > > > > > > You will also need to define the _ as the gettext function at the top of > > > this file with: > > > const _ = imports.gettext.gettext; > > > > > > @@ +74,3 @@ > > > this._buttonImage.icon_name = 'list-remove-symbolic'; > > > this._icon.icon_name = 'maps-point-end-symbolic'; > > > + this._button.set_tooltip_text("Remove in-route location"); > > > > > > Like above… > > > > > > @@ +79,3 @@ > > > this._buttonImage.icon_name = 'route-reverse-symbolic'; > > > this._icon.icon_name = 'maps-point-end-symbolic'; > > > + this._button.set_tooltip_text("Reverse Route"); > > > > > > I think we should probably not use title case for tooltip texts, so "Reverse > > > route" > > > > Thanks for helping me out with this. I've made the changes to the code and > > I'll follow the proper commit guidelines in my next patch. But I'm stuck > > with how to mark the string translatable. > > > I forgot to mention that this file also needs to be added to the list in > po/POTFILES.in > You could look in src/transitLegRow.js for an example of how to > programmatically set tooltip texts on buttons (around line 108). > And the _("...") wrapper is what makes the string marked for translation (it actually invokes the gettext function to lookup a translation for that string (that's what the imports.gettext.gettext defines). > > Also, can you please guide me to some good documentation so that I can > > understand the code on my own as it would help me with solving other bugs. > > We don't currenly have any good overall architectural overview of the source > (we probably should, though). But at least some is at > https://wiki.gnome.org/Apps/Maps/Resources, such as links to GJS docs.
Created attachment 366997 [details] [review] Tooltips to route planner buttons
Created attachment 366998 [details] [review] routeEntry: Add tooltips to route planner buttons
Created attachment 367042 [details] [review] routeEntry: Add tooltips to route location buttons
Review of attachment 366997 [details] [review]: LGTM (the patch wasn't formatted correctly as a git patch and tagged with the bug URL, but I fixed that)
Comment on attachment 367042 [details] [review] routeEntry: Add tooltips to route location buttons Attachment 367042 [details] pushed as 3baa45e - routeEntry: Add tooltips to route location buttons
(In reply to Marcus Lundblad from comment #31) > Review of attachment 366997 [details] [review] [review]: > > LGTM (the patch wasn't formatted correctly as a git patch and tagged with > the bug URL, but I fixed that) Thanks a lot. :)