Conversation
|
The new visual appearance of the menu button looks good to me. It looks like the modifications in the "buttonwithmodal" component offcentered the text of the confirmation dialog. The menu in the drawing mode has also a "close menu" button that should match the style of the main menu button. |
src/utils/ButtonWithIcon.vue
Outdated
| className = this.iconsBeforeText ? 'mt-2' : 'mb-2' | ||
| } else { | ||
| className = this.iconsBeforeText ? 'ms-1' : 'me-1' | ||
| className = this.iconsBeforeText ? 'ms-2' : 'me-2' |
There was a problem hiding this comment.
I have the impression that this pushes the text a bit off center, we should either compensate on the other side or go back to a smaller margin.
What was your goal by setting a bigger margin here?
There was a problem hiding this comment.
The goal is to have a margin between icon and label, not have them too close
Before:
After:
The issue also mentioned by @jedef is that the ButtonWithIcon class can be used without icon ! It is not very user friendly. I'll quickly fix this but we should consider some rework here to have the class either renamed and only used with icon. IMHO a button without icon doesn't require a component as we use bootstrap.
There was a problem hiding this comment.
By the way this of center was already present, just less bigger as it used a me-1 instead of me-2.
There was a problem hiding this comment.
the goal having a single component to manage the button was specifically to encapsulate bootstrap usage in one place, so that if we wanted to switch to something else, or tweak the design of existing buttons, it would only need a change in one place.
0ea6209 to
6b73d56
Compare
jedef
left a comment
There was a problem hiding this comment.
It seems like you have forgotten to change this button:
I would also suggest to define a global css class (e.g. "default-button") that includes the needed bootstrap classes "btn" and "btn-light" and use this class instead in the code. This would make the code less tightened to bootstrap, make it easier to know which button class to use when writing new code and make it easier to change the styles of all our buttons in one go if we want to.
6b73d56 to
a941089
Compare
|
@pakb I corrected as discussed using |
Ok we can do it in another ticket then (or in multiple, among others one for the buttons, as we do not necessarily need to switch all at once). The rest looks good to me now. |
a941089 to
9337934
Compare
|
@pakb after rebasing to dev I did some small improvement see latest commits. I also created a ticket https://jira.swisstopo.ch/browse/BGDIINF_SB-2680 to solve the button jungle after MVP ;-) |
Removed the top corner of the open/close menu button and increase a bit spaces.
…drawing menu styling Some button without icons where using the ButtonWithIcon class and therefore a margin to the label was added putting the text of center. To solve this I made the icon required in the later class and changed all button that don't have an icon to use directly bootstrap. Also align the style of the drawing open/close menu to the one of the regular menu.
The button had a warning and the selection was not kept.
Add some small margin in the time selector.
This button uses the PopOverButton which uses the ButtonWithIcon which had a fixed margin for the text. The issue is that the time selector button has no icon, therefore we don't need any margin. This has been temporarly fixed but we need to refactor the whole application button (see BGDIINF_SB-2680)
7e0a279 to
470039a
Compare
Removed the top corner of the open/close menu button and increase a bit spaces.
Test link