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

Initial review #1

Open
amercader opened this issue Jul 22, 2022 · 6 comments
Open

Initial review #1

amercader opened this issue Jul 22, 2022 · 6 comments

Comments

@amercader
Copy link
Member

Great work @avdata99, some things I found worth fixing before an initial release

  • Form styling, is this supposed to look like this? (I'm using a vanilla CKAN):
    Screenshot 2022-07-22 at 13-03-26 Administration - CKAN

  • Banner styling; this also doesn't look good on standard CKAN:
    Screenshot 2022-07-22 at 13-38-38 Administration - CKAN

  • If dates are mandatory they should be a validation step to check if there were filled. Now you get an Internal server error. Alternatively, not entering any date means that the announcement is shown immediately and indefinitely (until a sysadmin deletes it)

  • Small typo here:
    Screenshot 2022-07-22 at 13-39-09 Administration - CKAN

  • People will most likely want to override the announcement template so move it to a separate snippet in templates/announcements/announcement.html or something and then include that from templates/header.html to make it easier

  • Let's support Markdown styling by adding h.render_markdown(announcement.message) to the snippet

  • On the delete popup: "Are you sure you want to delete this announcement?", Scope -> Time Period

Also as this is a new extension let's run Black and use it going forward

@avdata99
Copy link
Member

Typo fixed

@avdata99
Copy link
Member

Create separate snippet for announcements here

@avdata99
Copy link
Member

Markdown allowed here

@avdata99
Copy link
Member

Updated delete pop up here

image

@avdata99
Copy link
Member

Flake8 added here
Should we also add black?

@amercader
Copy link
Member Author

Yes, let's use black @avdata99
Send a PR with further changes so they are easier to review

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

No branches or pull requests

2 participants