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 document title to all routes #3195

Merged
merged 4 commits into from
Feb 23, 2022
Merged

Add document title to all routes #3195

merged 4 commits into from
Feb 23, 2022

Conversation

dit7ya
Copy link
Contributor

@dit7ya dit7ya commented Feb 16, 2022

Description

2022-02-17_18-05

New stuff

Adds document title to all routes. Introduces a useTitle hook that gets called in the top-level Routes.tsx component.

TODOs

  • add a fallback strategy

Note to Reviewers

I have tried to ensure there is no weird edge-cases but please let me know if you find one or even suspect one.

Resolves #3036.

@dit7ya dit7ya self-assigned this Feb 16, 2022
@dit7ya dit7ya added the feature label Feb 16, 2022
@dit7ya dit7ya marked this pull request as draft February 16, 2022 13:47
@dit7ya dit7ya force-pushed the feat/3036-page-titles branch 2 times, most recently from 781d4ac to c8f2875 Compare February 17, 2022 12:36
@dit7ya dit7ya requested a review from a team February 17, 2022 12:38
@dit7ya dit7ya marked this pull request as ready for review February 17, 2022 12:38
Copy link
Member

@arrenv arrenv left a comment

Choose a reason for hiding this comment

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

@dit7ya it looks to be working really well, and I think showing the ENS name while waiting for the ColonyName query is a good idea.

The only change that needs to be made is that the transactions need to be more descriptive. As of this format "<ACTION/MOTION_TITLE> | Colony - <COLONY_NAME>"

So, instead of having Transaction - 0x259c9648ee907e9c4fb2a3f0f562e6d70c5851d586487150c38b18c1ce827df5 | Colony - CoolDAO it should be Pay arren 4950 CLO | Colony - CoolDAO or Pay arren 4950 CLO | Motion | Colony - CoolDAO

.
page-titles

@dit7ya dit7ya force-pushed the feat/3036-page-titles branch from c8f2875 to 67552ae Compare February 17, 2022 17:28
@dit7ya dit7ya marked this pull request as draft February 17, 2022 17:35
@dit7ya dit7ya force-pushed the feat/3036-page-titles branch 2 times, most recently from 66063bb to 5f53785 Compare February 17, 2022 19:34
@dit7ya dit7ya marked this pull request as ready for review February 17, 2022 19:36
@dit7ya dit7ya requested a review from a team February 18, 2022 09:39
@arrenv
Copy link
Member

arrenv commented Feb 18, 2022

@dit7ya It is looking good.

It seems the format changes for transaction pages. They are showing as: Pay arren 4950 CLO | Motions - CoolDAO | Colony and Pay arren 4950 CLO | Actions - CoolDAO | Colony.

It should be Pay arren 4950 CLO | Motion | Colony - CoolDAO and Pay arren 4950 CLO | Action | Colony - CoolDAO.

Screenshot 2022-02-18 112343

@dit7ya dit7ya force-pushed the feat/3036-page-titles branch from 5f53785 to d8cd211 Compare February 18, 2022 11:04
@dit7ya
Copy link
Contributor Author

dit7ya commented Feb 18, 2022

@arrenv. Thanks, fixed those now.

@arrenv
Copy link
Member

arrenv commented Feb 18, 2022

@dit7ya I think the last thing is to update the action one to match the motion page. I.e. | Colony - CoolDAO as opposed to | CoolDAO - Colony.

Screenshot 2022-02-18 123159

@dit7ya dit7ya force-pushed the feat/3036-page-titles branch from d8cd211 to 62e5ce0 Compare February 18, 2022 12:04
@dit7ya
Copy link
Contributor Author

dit7ya commented Feb 18, 2022

@arrenv. Oops, somehow missed that. Hopefully everything is fine now.

Copy link
Contributor

@ArmandoGraterol ArmandoGraterol left a comment

Choose a reason for hiding this comment

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

It seems like the majority of the titles look good, but it seems like the coin machine's title needs a bit more work:

Screenshot from 2022-02-18 15-21-02

As per the screenshot, for me, it's showing everything on under case, and with the colonyName not filled in (Most likely because it's colonyname but it actually needs to be colonyName)

@dit7ya dit7ya force-pushed the feat/3036-page-titles branch from 62e5ce0 to 10a51a3 Compare February 18, 2022 18:57
@dit7ya
Copy link
Contributor Author

dit7ya commented Feb 18, 2022

Thanks @ArmandoGraterol. Seems like somehow the command for lowercasing got ran on that line on my editor. Fixed it now.

Copy link
Contributor

@ArmandoGraterol ArmandoGraterol left a comment

Choose a reason for hiding this comment

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

Yup, that was the only thing I could find, all the other pages that I saw were looking good! Nice job

@arrenv
Copy link
Member

arrenv commented Feb 21, 2022

@dit7ya it is looking really good. A big improvement in my opinion.

This is the last thing I noticed, I think we could update VotingReputation to Motions and Disputes. What do you think?

download (15)

@dit7ya dit7ya force-pushed the feat/3036-page-titles branch from 10a51a3 to 67e0ce8 Compare February 22, 2022 06:18
@dit7ya
Copy link
Contributor Author

dit7ya commented Feb 22, 2022

Thanks @arrenv. While I was at it, I also made the other extensions page title look slightly better (like OneTxPayment to One Transaction Payment).

2022-02-22_11-41

Copy link
Contributor

@chinins chinins left a comment

Choose a reason for hiding this comment

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

Looks good, nice work! The only concern I have is a about translation ids in the useTitle hook (please, see my separate comment).


const MSG = defineMessages({
connect: {
id: 'routeTitles.connect',
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure that these should be the ids. I would think it should be utils.hooks.useTitle.connect as we give it based on the folder location and file name. Here is an example from the BuyTokens component:
id: 'dashboard.CoinMachine.BuyTokens.title'

I would double check with Raul...

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 agree with you. I was a little confused since there was no other example of messages being inside utils hooks. I will change them quick.

@dit7ya dit7ya force-pushed the feat/3036-page-titles branch from 67e0ce8 to 6113f5a Compare February 22, 2022 13:43
Copy link
Member

@arrenv arrenv left a comment

Choose a reason for hiding this comment

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

@dit7ya everything seems to be looking good! Nice one!

Copy link
Contributor

@chinins chinins left a comment

Choose a reason for hiding this comment

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

Good to go - nice work!

@dit7ya dit7ya force-pushed the feat/3036-page-titles branch from 6113f5a to 3120ece Compare February 23, 2022 15:38
@dit7ya dit7ya merged commit acab66b into master Feb 23, 2022
@dit7ya dit7ya deleted the feat/3036-page-titles branch February 23, 2022 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic page titles throughout Dapp routes
4 participants