Skip to content
This repository has been archived by the owner on Dec 28, 2021. It is now read-only.

Visual improvements 2 #1419

Merged
merged 19 commits into from
Apr 2, 2021
Merged

Conversation

MichaelMauderer
Copy link
Contributor

@MichaelMauderer MichaelMauderer commented Apr 1, 2021

Pull Request Description

More Visual improvements

image

Peek.2021-04-01.14-41.mp4

Checklist

Please include the following checklist in your PR:

  • The CHANGELOG.md was updated with the changes introduced in this PR.
  • The documentation has been updated if necessary.
  • All code conforms to the Rust style guide.
  • All code has automatic tests where possible.
  • All code has been profiled where possible.
  • All code has been manually tested in the IDE.
  • All code has been manually tested in the "debug/interface" scene.
  • All code has been manually tested by the PR owner against our test scenarios.
  • All code has been manually tested by at least one reviewer against our test scenarios.

@MichaelMauderer MichaelMauderer marked this pull request as ready for review April 1, 2021 11:25
src/rust/ensogl/lib/components/src/list_view.rs Outdated Show resolved Hide resolved
/// The overall entry's height (including padding).
pub const HEIGHT:f32 = 30.0;
/// The text size of entry's labe.
pub const LABEL_SIZE:f32 = 12.0;
/// The size in pixels of icons inside entries.
pub const ICON_SIZE:f32 = 0.0; // TODO[ao] restore when created some icons for searcher.
/// The gap between icon and label.
pub const ICON_LABEL_GAP:f32 = 7.0;
pub const ICON_LABEL_GAP:f32 = 0.0; // TODO[ao] restore when created some icons for searcher.
Copy link
Member

Choose a reason for hiding this comment

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

What does the sentence mean? "Restore" means to do something in the future. While "when created" is in the past. I don't understand it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied the comment from above. The searcher was at some point meant to contain icons. It does not at the moment, so these paddings sand gaps need to be 0 now, but they need to be set when we add icons. I've fixed the grammar, now.

src/rust/ensogl/lib/text/src/component/area.rs Outdated Show resolved Hide resolved
src/rust/ensogl/lib/theme/src/lib.rs Outdated Show resolved Hide resolved
src/rust/ensogl/lib/theme/src/lib.rs Outdated Show resolved Hide resolved
@@ -275,7 +285,7 @@ define_themes! { [light:0, dark:1]
highlight = selection , selection;
text = Lcha(0.0,0.0,0.0,0.7) , Lcha(1.0,0.0,0.0,0.7);
text {
highlight = Lcha(0.8,0.0,0.0,1.0) , Lcha(0.7,0.0,0.0,1.0);
highlight = Lcha(0.7,0.4,0.74,1.0) , Lcha(0.7,0.4,0.74,1.0);
Copy link
Member

Choose a reason for hiding this comment

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

do not hardcode values, link them to other places in the stylesheet if possible. (see my previous comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we have a matching value here. This is for the blue highlight in the searcher.

Comment on lines 91 to 92
// Note: Disabled for https://github.com/enso-org/ide/issues/1397
// Re-enable when they are needed again.
Copy link
Member

Choose a reason for hiding this comment

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

This comment tells nothing about this change. It just tells that this is "dsiabled" and was disabled as part of "visual improvements" and it should be re-enabled "when needed again". Such comments should be descriptive, in particular - why it is disabled (because these icons were useless now) and what does it mean "needed again" - when icons willbecome implemented properly + it should have link to issue about that (fixing / finishing the icons skip + freeze)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found the related issue and linked it. Expanded the description.

src/rust/ensogl/lib/components/src/list_view.rs Outdated Show resolved Hide resolved
@wdanilo wdanilo merged commit 1fa34b0 into develop Apr 2, 2021
@wdanilo wdanilo deleted the wip/mm/ide-1397-visual-improvements-2 branch April 3, 2021 07:03
mwu-tow pushed a commit to enso-org/enso that referenced this pull request Oct 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants