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 page specific head title #3526

Merged
merged 7 commits into from
May 25, 2021
Merged

Conversation

tuliren
Copy link
Contributor

@tuliren tuliren commented May 21, 2021

What

  • This is one of the few tiny UI tweaks I'd like to squash after following the tutorials.
  • Currently the web application always show the same HTML title on the browser toolbar.
  • This add page specific HTML title for each page.
  • Here is the comparison of the page history:
Current Airbyte Page History New Airbyte Page History

How

  • Helmet is used to update HTML head title in each page.

Pre-merge Checklist

  • Run integration tests
  • Publish Docker images

Recommended reading order

  1. HeadTitle.tsx
  2. the rest

┆Issue is synchronized with this Asana task by Unito

@tuliren tuliren requested review from jrhizor and jamakase May 21, 2021 08:31
@auto-assign auto-assign bot requested review from cgardens and sherifnada May 21, 2021 08:31
@tuliren tuliren requested review from avaidyanatha and removed request for sherifnada and cgardens May 21, 2021 08:33
@tuliren
Copy link
Contributor Author

tuliren commented May 21, 2021

Hmm, even though I have assigned the reviewers, the assign bot always tries to add two new people. This PR probably does not require that many pairs of eyes.

@davinchia
Copy link
Contributor

@tuliren feel free to remove people that you don't think are required

Copy link
Contributor

@avaidyanatha avaidyanatha left a comment

Choose a reason for hiding this comment

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

Can't speak to the TS, but this is a great idea for the UI.

Only comment would be perhaps using a instead of a | to separate subpages

Copy link
Contributor

@jamakase jamakase left a comment

Choose a reason for hiding this comment

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

I only have few questions, but in common looks fine. Awesome work!

airbyte-webapp/package.json Outdated Show resolved Hide resolved
airbyte-webapp/src/locales/en.json Outdated Show resolved Hide resolved
airbyte-webapp/src/pages/AdminPage/AdminPage.tsx Outdated Show resolved Hide resolved
@tuliren
Copy link
Contributor Author

tuliren commented May 22, 2021

Only comment would be perhaps using a • instead of a | to separate subpages

The pipe character | seems more clear and concise than . is not an ASCII character, and looks bulkier than the other English characters around it. So I'd prefer to stick with the pipe.

Screen Shot 2021-05-22 at 14 10 05

Screen Shot 2021-05-22 at 14 10 55

@avaidyanatha
Copy link
Contributor

Only comment would be perhaps using a • instead of a | to separate subpages

The pipe character | seems more clear and concise than . is not an ASCII character, and looks bulkier than the other English characters around it. So I'd prefer to stick with the pipe.

Fair enough! There is ASCII 250 which serves the same purpose, but the pipe is totally fine, just a stylistic suggestion.

@tuliren tuliren merged commit d24b5c5 into master May 25, 2021
@tuliren tuliren deleted the liren/add-page-specific-head-title branch May 25, 2021 04:40
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