-
Notifications
You must be signed in to change notification settings - Fork 19
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
Conversation
781d4ac
to
c8f2875
Compare
There was a problem hiding this 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
c8f2875
to
67552ae
Compare
66063bb
to
5f53785
Compare
@dit7ya It is looking good. It seems the format changes for transaction pages. They are showing as: It should be |
5f53785
to
d8cd211
Compare
@arrenv. Thanks, fixed those now. |
@dit7ya I think the last thing is to update the action one to match the motion page. I.e. |
d8cd211
to
62e5ce0
Compare
@arrenv. Oops, somehow missed that. Hopefully everything is fine now. |
There was a problem hiding this 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:
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
)
62e5ce0
to
10a51a3
Compare
Thanks @ArmandoGraterol. Seems like somehow the command for lowercasing got ran on that line on my editor. Fixed it now. |
There was a problem hiding this 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
@dit7ya it is looking really good. A big improvement in my opinion. This is the last thing I noticed, I think we could update |
10a51a3
to
67e0ce8
Compare
Thanks @arrenv. While I was at it, I also made the other extensions page title look slightly better (like |
There was a problem hiding this 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).
src/utils/hooks/useTitle.ts
Outdated
|
||
const MSG = defineMessages({ | ||
connect: { | ||
id: 'routeTitles.connect', |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
67e0ce8
to
6113f5a
Compare
There was a problem hiding this 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!
There was a problem hiding this 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!
6113f5a
to
3120ece
Compare
Description
New stuff ✨
Adds document title to all routes. Introduces a
useTitle
hook that gets called in the top-levelRoutes.tsx
component.TODOs
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.