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

Fixing error from Climatic maps> Themes background #8383

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

anastasia-mbithe
Copy link
Contributor

Fixes #8345
This is ready for review.
Though I didn't manage to test this since I didn't get any shapefile data with the geometry parameter, maybe @NanyanziAlice can do the first test.

@rdstern
Copy link
Collaborator

rdstern commented Jun 21, 2023

@NanyanziAlice have you been able to check this?

@NanyanziAlice
Copy link
Collaborator

@rdstern
Yes, I did check it and it is now working fine. However, there was an issue that cropped up not necessarily an error but rather a concern about the user's experience when they leave the fill parameter of the panel. border unchecked which is set by default to white in the classic and grey themes so whatever is inside the panel such as a map would not be visible. Anastasia and I would appreciate an opportunity to chat about it if necessary.

@N-thony
Copy link
Collaborator

N-thony commented Jul 24, 2023

@rdstern Yes, I did check it and it is now working fine. However, there was an issue that cropped up not necessarily an error but rather a concern about the user's experience when they leave the fill parameter of the panel. border unchecked which is set by default to white in the classic and grey themes so whatever is inside the panel such as a map would not be visible. Anastasia and I would appreciate an opportunity to chat about it if necessary.

@anastasia-mbithe is this resolved now?

@anastasia-mbithe
Copy link
Contributor Author

Hi @N-thony, yes, for this issue it was done. The other was a different discussion/issue we wanted to get Roger's view, still on the same dialog.

@N-thony
Copy link
Collaborator

N-thony commented Jul 26, 2023

Hi @N-thony, yes, for this issue it was done. The other was a different discussion/issue we wanted to get Roger's view, still on the same dialog.

Ok, @derekagorhom can you peer review this? Thanks

@derekagorhom
Copy link
Contributor

@anastasia-mbithe
the changes to the panels work fine but the map ends up not being visible to the user. i think Alice raised that concern when she did a review and its still the same.

@N-thony
Copy link
Collaborator

N-thony commented Aug 28, 2023

@derekagorhom are you happy to take this over? and probably discuss with @anastasia-mbithe on what is needed/remaining here?

@derekagorhom
Copy link
Contributor

@N-thony sorry for bring this here on this PR, but since this PR attempts to fix the theme issue i felt here so better.
It seems Panel.border is the reason why the maps are not visible
image

Here is when you fill the panel.border option
with panel

Here is when you dont fill the panel.border option
Without panel

Is it ok if we disable the panel.border option for the user?

@Patowhiz
Copy link
Contributor

@derekagorhom does this require @rdstern input?

@derekagorhom
Copy link
Contributor

@Patowhiz i think Roger can weight in,
I do not know what the Panel border is used for (I have never used it). but if it is in regards to theme background then the panel background is the one responsible for the customization.

@Patowhiz
Copy link
Contributor

@derekagorhom thanks for your willingness to take on this task.

Having reviewed the issue and PR, it appears to be primarily a technical matter that doesn't need @rdstern's involvement at this stage.

It's my belief that completely disabling the feature without a comprehensive grasp of the underlying problem might not be the most prudent approach.

I would like to suggest that you delve into the R ggplot package, paying particular attention to the panel options, to gain a deeper understanding of why the script generated by the sub-dialog is triggering the issue described. Subsequently, I encourage you to scrutinize the alterations implemented in this PR to assess if they indeed address the issue in the most appropriate manner. If these changes prove inadequate, I suggest you create a new PR that attempts to fix the issue.

Your insights and feedback on this matter would be greatly valued as they will contribute to our progress.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Themes Background options throws an error
6 participants