Skip to content
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

Features/stack labels #109

Merged
merged 30 commits into from
Apr 4, 2023
Merged

Conversation

ArmelVidali
Copy link
Contributor

@ArmelVidali ArmelVidali commented Mar 24, 2023

Hello,

We added a functionnality together with @robLittiere and @DevGuez allowing to automatically stack labels one underneath the other when multiple labels layers are on the map, taking into account the presence or absence of pictogram layer.

Before, when multiple labels were on the map, they were stacked on top of each other.

It stacks the labels one uderneath the other by adding to their "y" coordinate the height of the image to which the label corresponds (if it is present on the map) and the font size of other labels.

This feature was made in order to work with our previous pull request so we added a list of checkbox to the menu allowing to select images and filter them in order to select labels at the same time

label_from_picto

We are unsure about your preferences concerning user interface changes, please let us know if you would like magrit interface to stay the same, but this feature works as well when adding labels one by one from the layer menu as below, still taking into account the presence of images.

label_from_layer

It still works when saving and loading magrit project files.

@ArmelVidali ArmelVidali deleted the features/stack_labels branch March 24, 2023 13:21
@ArmelVidali ArmelVidali restored the features/stack_labels branch March 24, 2023 13:21
@ArmelVidali
Copy link
Contributor Author

I made some errors modifying some files and will rewrite it again

@mthh
Copy link
Member

mthh commented Mar 24, 2023

Hello @ArmelVidali,

Being able to stack labels layers between them and with pictogram layers sounds great ! 🎆

On the other hand, the possibility of adding the labels at the same time as the pictograms is a strong choice, I have no opinion on this yet (I'll discuss it with @rCarto - knowing that we're trying to keep the interface as simple as possible).

Count on me to review your PR when the time comes (and don't hesitate to ask for guidance if needed, or even to discuss the design of specific aspects before implementing them).

@ArmelVidali
Copy link
Contributor Author

Thanks for the feedback ! I am not very used to git and github usage, I thought that i modified files unnecessarily and hence closed the request but everything seems fine, except for the "test" commit adding a point to app.py, I will reopen the request now.

We would be glad to get some guidelines since we'll keep working on this project !

@ArmelVidali ArmelVidali reopened this Mar 24, 2023
@mthh
Copy link
Member

mthh commented Mar 27, 2023

Could you rebase your branch against the current state of riatelab/magrit master branch ?

@ArmelVidali
Copy link
Contributor Author

It's done, it seems to be the right version, please let me know

@mthh
Copy link
Member

mthh commented Mar 28, 2023

I tested it, it seems to work well and it's a really good idea!

However, I have some doubts on 2 main points:

  • The labels are created by default in Verdana font with size 12. It is likely that the user will want to customize this later, which could lead to new overlays between the labels.

  • Concerning the pictograms + labels menu, I think that it makes the window quite heavy and I think I would prefer to keep the menu with only the pictograms.

But these two points lead me to think that we should improve the current label creation menu (it is currently rather rustic, cf. your second screenshot) to allow the user to choose the font and its size at the time of creation, as well as to allow the creation of several layers of labels at once, as you have implemented in the pictogram + label menu.

One could even imagine that the user could choose the size and font individually for each layer during creation (see example screenshot).
In this way, the user could benefit from your "label stacking" functionality with the labels at the right size.

fake-menu

To sum up, currently I'm thinking of: keeping your functionality for stacking labels, but not changing the pictogram menu.
Then (this doesn't have to happen in this PR and I can take care of it a few days after merging this PR) improve the current label creation window.

What do you think ?
@rCarto, any thoughts?

@ArmelVidali
Copy link
Contributor Author

ArmelVidali commented Mar 28, 2023

I agree it's much better to have a detailed menu when adding labels from the layer itself

I initially put it in the pictogram panel because I thought it could be a good idea to have this functionnality for the proportionnal point representation as well (stock, stock and ratio, stock and qualitative, and probably waffle map but I can't find a layer for which this one works on magrit and i'm not sure what it looks like), so that the labels be stacked on any ponctual layer.
Adding this detailed menu to each representation won't look good

Deciding under which ponctual layer it must be stacked (for example an image and a proportionnal point are displayed over the same entity) might be tricky, but I think it could work by choosing the largest, like in this example.

image

Do you think it'd be a good idea ?

As of changing the label menu from the layer I think it would look better if you added it yourself indeed

@mthh
Copy link
Member

mthh commented Mar 29, 2023

Indeed, it could be interesting to be able to stack label layers above / below any punctual layer! (and the strategy of choosing the largest totally makes sense I guess)

However maybe the user should be able to choose if he/she want to opt-in or not for this functionality because I can also see cases when stacking the label with an other punctual layer is probably not what is expected (cf. screenshot).

Capture du 2023-03-29 14-09-33

@ArmelVidali
Copy link
Contributor Author

Yes I think it's a good idea, it could be implemented by a simple checkbox, by stacking underneath by default for images and on the center by default for prop symbols.

I will look into that !

@ArmelVidali
Copy link
Contributor Author

After discussing with @robLittiere, we thought it might be a better idea to keep the stack labels functionnality isolated from the positionning of the labels depending on the presence of proportional symbol layer.

We thought we could add the other options in subsequents pull requests.

Concerning this PR, would you rather have us only keep the label stacking feature or include as well the other options like taking into account the presence of proportional symbol layer ?

@mthh
Copy link
Member

mthh commented Apr 3, 2023

Hello @ArmelVidali,

No problem, let's focus on the label stacking feature in this PR !

If I'm not mistaken, it already seems to be in good shape and almost ready to be merged? Could you just remove what you had added to the pictograms choice window?

My plan is the following:

  • merge your contribution about stacking labels,
  • modify the label creation window (as mentioned above),
  • publish a new version with these two changes.

…options to stack under prop symbols. Removed render_multiple_label function.
@ArmelVidali
Copy link
Contributor Author

It's done ! I removed as well the few unrelated lines

display: flex;
flex-direction: column;
}
/* End of styles for list of label elements in categorical / picto panel */
Copy link
Member

@mthh mthh Apr 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the changes in this file still usefull ?

@ArmelVidali
Copy link
Contributor Author

This insn't usefull indeed, i removed it

@mthh
Copy link
Member

mthh commented Apr 4, 2023

Thanks a lot !

@mthh mthh self-requested a review April 4, 2023 09:14
@mthh mthh merged commit 88c2825 into riatelab:master Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants