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

Add tooltips to app bar #657

Merged
merged 3 commits into from
Nov 30, 2023
Merged

Add tooltips to app bar #657

merged 3 commits into from
Nov 30, 2023

Conversation

rolfheij-sil
Copy link
Contributor

@rolfheij-sil rolfheij-sil commented Nov 30, 2023

Fixed #515

This change is Reviewable

@Sebastian-ubs
Copy link
Contributor

Screenshot from paranext/paranext-extension-text-collection#25 looks hard to read due to the opacity. I suggest opacity should be removed.

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Awesome! Just one little thing and a couple suggestions. Thanks for the quick snipe on this! It will be nice to have this PR as a concise guide for adding things here in the future :)

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @rolfheij-sil)


src/renderer/components/docking/platform-tab-title.component.tsx line 26 at r1 (raw file):

  };

  const tooltipDiv = tooltip ? <div style={{ whiteSpace: 'pre-line' }}>{tooltip}</div> : '';

Please make a class for this if possible :)


src/renderer/components/docking/platform-tab-title.component.tsx line 26 at r1 (raw file):

  };

  const tooltipDiv = tooltip ? <div style={{ whiteSpace: 'pre-line' }}>{tooltip}</div> : '';

I think it would be nice to let extension devs use spaces and tabs at the start of their lines if they want - what do you think about using pre-wrap instead? Then they could make indented lines and such if desired.


src/shared/models/web-view.model.ts line 42 at r1 (raw file):

  /** Name of the tab for the WebView */
  title?: string;
  /** Tooltip than is shown when hovering over the webview title */

typo? "than" -> "that"

Copy link
Contributor Author

@rolfheij-sil rolfheij-sil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tjcouch-sil)


src/renderer/components/docking/platform-tab-title.component.tsx line 26 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Please make a class for this if possible :)

Done.


src/renderer/components/docking/platform-tab-title.component.tsx line 26 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think it would be nice to let extension devs use spaces and tabs at the start of their lines if they want - what do you think about using pre-wrap instead? Then they could make indented lines and such if desired.

Done


src/shared/models/web-view.model.ts line 42 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

typo? "than" -> "that"

Well spotted. Fixed.

tjcouch-sil
tjcouch-sil previously approved these changes Nov 30, 2023
Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

:lgtm: thanks!!

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@rolfheij-sil rolfheij-sil merged commit 4e47845 into main Nov 30, 2023
7 checks passed
@rolfheij-sil rolfheij-sil deleted the 515-webview-tooltips branch November 30, 2023 16:06
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.

Webview tooltips
3 participants