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

feat: add status tooltips #32011

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

Conversation

iterminatorheart
Copy link
Contributor


  • There are tests for these changes OR
  • These changes do not require tests because ___

ports/servoshell/webview.rs Outdated Show resolved Hide resolved
@sagudev sagudev requested a review from delan April 7, 2024 19:50
@iterminatorheart iterminatorheart force-pushed the feat/status_tooltip branch 4 times, most recently from 140ebe8 to 35ce190 Compare April 10, 2024 16:13
Copy link
Sponsor Member

@delan delan left a comment

Choose a reason for hiding this comment

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

This is coming along well! A few comments:

@@ -48,6 +48,8 @@ pub struct Minibrowser {
location_dirty: Cell<bool>,

load_status: LoadStatus,

hovered_link: Option<String>,
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

recommend renaming this to status_text to make it more clearly related to EmbedderMsg::Status, and avoid confusion if we later show other kinds of status bar text

@@ -371,6 +391,15 @@ impl Minibrowser {
need_update
}

pub fn update_hovered_link(
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

same here

@@ -37,6 +37,7 @@ use crate::window_trait::{WindowPortsMethods, LINE_HEIGHT};
pub struct WebViewManager<Window: WindowPortsMethods + ?Sized> {
current_url: Option<ServoUrl>,
current_url_string: Option<String>,
hovered_link: Option<String>,
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

same here

@@ -130,6 +132,10 @@ where
self.load_status
}

pub fn hovered_link(&self) -> Option<String> {
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

same here

Comment on lines +225 to +227
TopBottomPanel::bottom("innner_hover_links")
.max_height(0.0)
.show(ctx, |ui| {
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

this can be done without a TopBottomPanel by replacing ui.cursor().max.y with ctx.available_rect().max.y

@@ -217,6 +220,23 @@ impl Minibrowser {
return;
};
let mut embedder_events = vec![];

if self.hovered_link.is_some() {
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Suggested change
if self.hovered_link.is_some() {
if let Some(status_text) = &self.status_text {

and then do ui.label(status_text.clone());

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.

servoshell: show a status bar tooltip when hovering over links
3 participants