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

feat: implement proposal review page #346

Merged
merged 3 commits into from
Oct 11, 2024

Conversation

Zied-Dahmani
Copy link
Collaborator

@Zied-Dahmani Zied-Dahmani commented Oct 9, 2024

Scope

Ticket

I have implemented the proposal creation (review and publish) page UI.

Screenshots

@Zied-Dahmani Zied-Dahmani requested a review from n13 October 9, 2024 13:40
@Zied-Dahmani Zied-Dahmani self-assigned this Oct 9, 2024
@Zied-Dahmani Zied-Dahmani linked an issue Oct 9, 2024 that may be closed by this pull request
Copy link
Collaborator

@n13 n13 left a comment

Choose a reason for hiding this comment

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

Just some questions - what are the view indices, and can they have names, or an enum instead? So we don't have magic numbers in the code?

final String? _text;

// TODO(Zied-Saif): figure this out
const ProposalHeader(this._dao, {String text = 'Marketing Circle', super.key}) : _text = text;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm why is Marketing Circle hard-coded here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is supposed to be the circle it could either be an empty string (no circle) - which should then be the default, or some other text if there is a circle selected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have no idea what the Marketing Circle is, so we let it default, created a ticket, and will get back to it later (I remember Luigi said we would remove it).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok please make the default "" empty string then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's OK to merge something that is clearly wrong. Principle. Just like we don't merge bugs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Empty string should be fine as default - some proposals will have a circle some won't (default is no circle)

@Zied-Dahmani Zied-Dahmani requested a review from n13 October 10, 2024 15:24
@Zied-Dahmani Zied-Dahmani merged commit 5a5fdd2 into main Oct 11, 2024
0 of 3 checks passed
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

Successfully merging this pull request may close these issues.

Proposal Creation (Review and Publish) UI
2 participants