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

Credits #122

Merged
merged 29 commits into from
Jan 1, 2022
Merged

Credits #122

merged 29 commits into from
Jan 1, 2022

Conversation

nathanfallet
Copy link
Member

@nathanfallet nathanfallet commented May 22, 2021

Description

I'm adding a view to show contributors, based on the backend contributors endpoint

Related issue

Fixes #37

What I tested

The view itself

Regression risks

N/A

Screenshots

If applicable, add screenshots to help explain the fix. Otherwise please remove this section
36045A3B-7D95-46A8-A707-97146A77A3D1-9891-000006EB55F5A69D

Checklist

  • I have installed SwiftLint and made sure code formatting is correct
  • I have performed a self-review of my own code
  • I tested my changes on real device
  • My changes generate no new warnings
  • I have commented my code in hard-to-understand areas
  • Any dependent changes have been merged into develop
  • I have checked there aren't other open Pull Requests for the same update/change

@nathanfallet nathanfallet mentioned this pull request May 26, 2021
@pylapp pylapp mentioned this pull request May 26, 2021
7 tasks
@victor-sarda victor-sarda added this to the 14.1 milestone Dec 19, 2021
@pylapp pylapp self-requested a review December 30, 2021 17:17
@pylapp pylapp modified the milestones: 1.4.1, 1.5.0 Dec 30, 2021
@pylapp
Copy link
Contributor

pylapp commented Dec 30, 2021

I tested your credits branch, here are some comments for the feature :)

About the contributors

  • For some contributors, we have a pseudonym of the fulll name, some details about contrebutions or not, and an avatar or not. Is it expected?
  • How are sorted the contributors? Maybe be should sort them by alphabetic order and by platforms
  • For Paul Jannot, an error occurred with it hyperlink. The hyperlink pauljeannot.github.io does not work anymore.
  • Same thing for CorentiOS with its hyperlink corentinmedina.fr
  • Same thing for Alexandre GUY with its hyperlink www.agraphik.fr
  • Same thing for Guillaume Rozier with its hyperlink guillaumerozier.fr

About the UI

  • We cannot see easily if we have a clickable cell because an hyperlink is defined, or not.
    Maybe we should add in the contributor cell an image button (containing an SF Symbol) which will redirect the user to the hyperlink target on tap

About a11y

  • We may setup a trick for vocalization of "ios" ; maybe convert the word to "iOS" or change the accessible label
  • Accessible fonts are not used in this view ; if we use bigger text sizes in device settings, the text size are not changed

Miscellaneous

  • Need to rebase your branch from develop :)
  • About KingFisher, register this library in the project (licenses folder) and the app (LicenseInfo.plist)
  • Some dead code to remove

ViteMaDose/Models/Credit.swift Outdated Show resolved Hide resolved
ViteMaDose/ViewModels/Credit/CreditViewModel.swift Outdated Show resolved Hide resolved
ViteMaDose/ViewModels/Credit/CreditViewModel.swift Outdated Show resolved Hide resolved
ViteMaDose/Views/Credits/CreditViewController.swift Outdated Show resolved Hide resolved
ViteMaDose/Views/Credits/Header/CreditHeaderView.swift Outdated Show resolved Hide resolved
ViteMaDose/Views/Credits/Header/CreditHeaderView.swift Outdated Show resolved Hide resolved
ViteMaDose/Views/Credits/Header/CreditHeaderView.swift Outdated Show resolved Hide resolved
ViteMaDose/Views/Credits/Header/CreditSectionView.swift Outdated Show resolved Hide resolved
@pylapp
Copy link
Contributor

pylapp commented Dec 30, 2021

@nathanfallet Almost done! The main things to review are about the accessibility (not managed yet in your feature), and also the ordering of the contributors. A bit of dead code too.

@nathanfallet
Copy link
Member Author

nathanfallet commented Dec 31, 2021

For some contributors, we have a pseudonym of the fulll name, some details about contributions or not, and an avatar or not. Is it expected?

Yes. The data is returned by the API at https://vitemadose.gitlab.io/vitemadose/contributors_all.json, so we have no control about it in the app, and for some of them not all data is filled.

Note: I opened this pull request to fix duplicates in the list

For Paul Jannot, an error occurred with it hyperlink. The hyperlink pauljeannot.github.io does not work anymore.
Same thing for CorentiOS with its hyperlink corentinmedina.fr
Same thing for Alexandre GUY with its hyperlink www.agraphik.fr
Same thing for Guillaume Rozier with its hyperlink guillaumerozier.fr

As the data is returned by the API, they need to correct the link where it was fetched from. (I think it was taken from GitHub API, so they need to fix the link in their GitHub profile)

Need to rebase your branch from develop :)

I started the process, it would take too long to do it, with all the changes since the original branch.

About KingFisher, register this library in the project (licenses folder) and the app (LicenseInfo.plist)

I let you work on that, you seemed motivated to, and know how to.

- Improve icon of hyperlink
- Display icon of hyperlink only if hyperlink defined and working
- Keep unique contirbutors

Signed-off-by: Pierre-Yves Lapersonne <dev@pylapersonne.info>
@pylapp
Copy link
Contributor

pylapp commented Dec 31, 2021

@nathanfallet I don't have completed this review and refactoring yet, but it's ongoing.
I hope to complete it this night or tomorrow.

I shared to mates the issue about not working hyperlink and doublons in the contributors JSON file.

Here are the things I will do a soon as possible:

  • Improve VoiceOver vocalization (hint for ios, hint for hyperlink icons)
  • Improve UI if too big text size in use
  • Add KingFisher in the project (details in app settings, licence in project)

I keep you in touch !

- Improve display of role with correct syntax
- Improve vocalization of roles
- Add label and hint for profil button

Signed-off-by: Pierre-Yves Lapersonne <dev@pylapersonne.info>
Signed-off-by: Pierre-Yves Lapersonne <dev@pylapersonne.info>
Signed-off-by: Pierre-Yves Lapersonne <dev@pylapersonne.info>
Signed-off-by: Pierre-Yves Lapersonne <dev@pylapersonne.info>
Please not a rebase operation must be preferd, but is to tricky: the feature branch was a stale branch and too much time has run.
Copy link
Contributor

@pylapp pylapp left a comment

Choose a reason for hiding this comment

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

Great!

@pylapp pylapp merged commit c027f37 into develop Jan 1, 2022
@pylapp pylapp deleted the credits branch January 1, 2022 17:49
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.

A view to show project contributors
3 participants