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

Add offerbook and marketprice domain #78

Merged

Conversation

HenrikJannsen
Copy link
Contributor

No description provided.

Copy link
Collaborator

@rodvar rodvar left a comment

Choose a reason for hiding this comment

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

tested in 3 apps, it works great (had an issue in iOS not related to this code, see matrix for details)

Besides my code review comments, I've also got a fleet warning with the .png assets.

there is an historic android reason why this happens, images filenames should always be lowercase and if using a separator it should be "_" as opposed to the one used in this PR "-" -> please change the file names to comply with compose assets rules.

That's it for now! I think we could probably merge this later today 💪

@HenrikJannsen
Copy link
Contributor Author

Review comments applied beside 2 items (see discussion)

@rodvar rodvar merged commit b548046 into bisq-network:main Nov 27, 2024
1 check passed
super.initializeServices()
private var applicationServiceCreated = false
override fun onViewAttached() {
if (!applicationServiceCreated) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Getting an error here when doing ./gradlew build.

This function needs to call super.onViewAttached()
Same for onViewUnattaching and onDestroying()

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep I've fixed that before merge buddha! 💪

@HenrikJannsen HenrikJannsen deleted the add-offerbook-and-marketprice-domain branch November 27, 2024 15:55
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