-
Notifications
You must be signed in to change notification settings - Fork 32
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
IGN - Organization Page #877
Conversation
Affected libs:
|
📷 Screenshots are here! |
Implements #772 |
cf6c0c2
to
aee0132
Compare
ad0c763
to
d69d487
Compare
eca730f
to
9e808d4
Compare
ee8ebf8
to
c692d1c
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.
Very nice work here ! I have left comments about vars names, leftover comments, and suggestions to improve the subscriptions, styling and fallbacks.
Also, some issues I found while testing :
- The mailing button doesn't work. When clicking on it, nothing happens - I have added a comment on the button's code because it's missing the target=blank.
- Add a fallback in case there is no email. For instance, the MEL org doesn't have an email (actually, a lot of the orgs don't so it might be worth checking if there isn't another source in the metadata for that where they might be?).
- As mentioned in the concerned component, if the user decides to look for a specific organization through the URL, there is no fallback page. Use the same fallback as "record not found". Here I tried with : http://localhost:4200/organization/1234
apps/datahub/src/app/home/news-page/key-figures/key-figures.component.html
Show resolved
Hide resolved
apps/datahub/src/app/organization/organization-details/organization-details.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/organization/organization-details/organization-details.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/organization/organization-details/organization-details.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/organization/organization-details/organization-details.component.spec.ts
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/organization/organization-details/organization-details.component.html
Outdated
Show resolved
Hide resolved
c692d1c
to
8f91bdb
Compare
a1ac3ff
to
10aa2d3
Compare
10aa2d3
to
b60d11f
Compare
b60d11f
to
48e0ce8
Compare
48e0ce8
to
756270f
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.
Thanks for the rework!
It looks better already 🙂
I just have noticed one behavior I mentioned in my last review that you might have missed that I'm copy pasting here :
"Add a fallback in case there is no email. For instance, the MEL org doesn't have an email (actually, a lot of the orgs don't so it might be worth checking if there isn't another source in the metadata where they might be?)." --> I think we should at least make the button disabled or something if the email is not available, it makes more sense user-wise.
Also, for you next PRs, it might be nice to keep the history of your commits instead of force-pushing on the same commit after a review has been done, it's easier for the reviewer to follow the changes made, IMO 😄
756270f
to
02f2ac2
Compare
02f2ac2
to
27816fd
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.
Thanks for the last fix, LGTM 😃
Description
This PR add a new Organization page.
### BREAKING CHANGES : I wanted to fix the typo : organisation -> organization. I shouldn't have maybe idk. But it may break things in the router. Be careful when rebasing forked repos.
Screenshots
Quality Assurance Checklist
breaking change
labelbackport <release branch>
labelThis work is sponsored by IGN.