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

Fix the UI of the spotter by outlining the input field. Fix pharo/#10993 #421

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

privat
Copy link
Contributor

@privat privat commented Sep 30, 2022

The input field is now light gray (or whatever according to the smalltalk theme), so it is obvious that the x cross belongs to the input field (and clears it) and does not belong to the whole modal window (and mistaken to a close button).

Done with @DanielCamSan

@Ducasse
Copy link
Contributor

Ducasse commented Nov 3, 2022

We should not use Smalltalk ui theme lightBackgroundColor but we should ask the stApplication because it is it that should manage the color. Now I do not know if the stApplication provides this (it was the idea) so

  • if stApplication does not we should introduce it probably proposing for now a dictionary key value for the role and the colors (colorFor: #lightBackgroundColor) but we should check how this is interacting with the style of the application (I never looked at the implementation).
  • if it does we should use it.
    @estebanlm ?

@Ducasse
Copy link
Contributor

Ducasse commented Nov 3, 2022

PS: Smalltalk ui theme lightBackgroundColor is a super bad pattern because this is a global variable accessed from everywhere and we want to kill them all.

@estebanlm
Copy link
Member

can you put a screenshot on how it looks in white and black themes?

@Ducasse
Copy link
Contributor

Ducasse commented Nov 3, 2022

@esteban does stApplication support theme color access?

@privat
Copy link
Contributor Author

privat commented Jan 19, 2023

I did just realize that this PR is still not merged. I agree that the proposed Smalltalk ui theme lightBackgroundColor is not ideal, but it is consistent with the current implementation where Smalltalk ui theme is already used in the same method https://github.com/pharo-spec/NewTools/blob/e831ff29bef86e6bf6c82c735cc26f7af24bb694/src/NewTools-Morphic-Spotter/StSpotterStyleContributor.class.st (and likely in other methods of the tool).

Moving to a better theme color approach is the right thing to do, but it could be done more globally in a future PR that tackle the whole tool.

So my current understanding is that the current PR is a (small) improvement that fixes an existing prevalent issue without introducing (new) technical dept.

@Ducasse
Copy link
Contributor

Ducasse commented Jan 19, 2023

I let estaban manage the integration. @estebanlm

Copy link
Contributor

@MarcusDenker MarcusDenker left a comment

Choose a reason for hiding this comment

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

I think we should merge this, we can always improve furtber

@MarcusDenker MarcusDenker merged commit df4dfc6 into pharo-spec:Pharo11 Apr 3, 2023
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.

5 participants