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

Integrate DAO UI #333

Merged
merged 37 commits into from
Nov 24, 2023
Merged

Integrate DAO UI #333

merged 37 commits into from
Nov 24, 2023

Conversation

jagracar
Copy link
Member

In this PR i will try to include the DAO UI into the teia page

Still a lot of things to be done

Feel free to modify at will

@jagracar jagracar requested review from xat, Zir0h and melMass September 26, 2023 01:04
@github-actions
Copy link

github-actions bot commented Sep 26, 2023

PR Preview Action v1.4.4
Preview removed because the pull request was closed.
2023-11-24 13:01 UTC

@jagracar
Copy link
Member Author

Hi. All needed things should be there.

The CSS is terrible and should be improved.

It should also use the teia form components in the proposal creation page, but i'm to confused to use them... Could anyone of you take care of that?

I think from this moment you can start to help me to do things better and nicer. I don't plan to touch this today anymore.

@melMass melMass marked this pull request as draft October 11, 2023 12:08
@jagracar jagracar marked this pull request as ready for review October 14, 2023 15:45
@jagracar
Copy link
Member Author

Hi @melMass @xat @Zir0h

My work is done. This PR has all that is needed for the DAO UI.
I tried to follow as much as possible the teia-ui code style, but problably i missed a couple of things.
Feel free to modify the code if you think something could be done better.

@jagracar
Copy link
Member Author

Ok, i lied...
While i was waiting for the review i started to work on the teia polls page. It took me very little to add it, so i decided to combine it with this PR, because they use the same code base and it would be nice to have both pages ready at the same time.

@melMass
Copy link
Member

melMass commented Oct 17, 2023

Ok, i lied... While i was waiting for the review i started to work on the teia polls page. It took me very little to add it, so i decided to combine it with this PR, because they use the same code base and it would be nice to have both pages ready at the same time.

I have a similar issue, I told everyone I was free at the same time 😅 !
Let's try to get it merged by next week! I will be able to dedicate a bit of time tomorrow morning and can't really predict when for the rest but I will be able to before next week

@jagracar
Copy link
Member Author

jagracar commented Oct 17, 2023

Ok, i lied... While i was waiting for the review i started to work on the teia polls page. It took me very little to add it, so i decided to combine it with this PR, because they use the same code base and it would be nice to have both pages ready at the same time.

I have a similar issue, I told everyone I was free at the same time 😅 ! Let's try to get it merged by next week! I will be able to dedicate a bit of time tomorrow morning and can't really predict when for the rest but I will be able to before next week

Great!
I will do some more changes today. I want to add a pages for each individual poll and DAO proposal, like we have for OBJKTs:

teia.art/polls/123
teia.art/dao/123

@jagracar jagracar marked this pull request as draft October 17, 2023 21:53
@jagracar jagracar marked this pull request as ready for review October 18, 2023 23:34
@jagracar
Copy link
Member Author

55 files changed or added. If you let me continue working in this PR i'm sure i can reach 100! XD

@melMass
Copy link
Member

melMass commented Oct 21, 2023

I'm merging back main now.
I can start the review tomorrow if it's ready @jagracar ?

@jagracar
Copy link
Member Author

I'm merging back main now. I can start the review tomorrow if it's ready @jagracar ?

Sure. Go for it!

Copy link
Member

@melMass melMass left a comment

Choose a reason for hiding this comment

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

All good for me, the only small improvements I can see but that aren't that critical:

  • I think we can merge SimpleInput and Input.
  • Some places use "raw" html buttons
  • We could extract the tables into a Table component
  • Claim should probably be removed now (we are the 21)
  • @jagracar has to update the contract urls once redeployed.

But overall it's well organised and can be merged once ready

Comment on lines +1 to +42
import styles from '@style'

export default function SimpleInput({
type,
label,
min,
max,
step,
minlength,
maxlength,
placeholder,
value,
onChange,
pattern,
className,
children,
}) {
const handleInput = (e) =>
onChange(type === 'file' ? e.target.files?.[0] : e.target.value)

return (
<div className={`${styles.container} ${className ?? ''}`}>
<label>
<p>{label}</p>
<input
type={type}
min={min}
max={max}
step={step}
minLength={minlength}
maxLength={maxlength}
placeholder={placeholder}
value={value}
onChange={handleInput}
pattern={pattern}
autoComplete="off"
/>
</label>
{children}
</div>
)
}
Copy link
Member

Choose a reason for hiding this comment

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

Do you remember why this was needed? I just tried replacing them all with the Input one and it seems to work fine but I remember you mentioned having issues with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had weird problems when i was using a map to create a list of inputs. For example, for the list of addresses in a tez transfer proposal

Copy link
Member

Choose a reason for hiding this comment

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

I see let's not break it now it could be refactored later!
Very very good job overall 👏

@jagracar
Copy link
Member Author

Thank you for the review, @melMass ! I think we could remove or update the claim page in another PR. We probably should also remove the banner that announced the claim process.

@melMass
Copy link
Member

melMass commented Nov 22, 2023

I think we could remove or update the claim page in another PR.

Unless it becomes too complex I would do it here

We probably should also remove the banner that announced the claim process.

Yep that's dealt with there, merch just did 😋 teia-community/teia-status@5f053ab

@jagracar
Copy link
Member Author

Working on it now. Should I remove everything? Or just the direct links to the claim page? It could be useful to keep the old code for future claim processes

@melMass
Copy link
Member

melMass commented Nov 22, 2023

Working on it now. Should I remove everything? Or just the direct links to the claim page? It could be useful to keep the old code for future claim processes

Yep I would keep it all just remove it's direct access from the menu

@jagracar
Copy link
Member Author

I redeployed the contracts and updated the UI. I think we can probably merge this tomorrow.

@jagracar
Copy link
Member Author

We will probably merge it tomorrow. I just need to click the "Squash and merge" button, right?

@melMass
Copy link
Member

melMass commented Nov 24, 2023

We will probably merge it tomorrow. I just need to click the "Squash and merge" button, right?

It's maybe better to rebase merge this one, it will be a massive commit otherwise

@jagracar jagracar merged commit b0bd1ca into main Nov 24, 2023
@jagracar jagracar deleted the feature/integrate-dao-pages branch November 24, 2023 13:00
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.

3 participants