Skip to content
This repository has been archived by the owner on Aug 2, 2020. It is now read-only.

fixed get_valid_macs and ui #45

Merged
merged 2 commits into from Sep 23, 2019
Merged

fixed get_valid_macs and ui #45

merged 2 commits into from Sep 23, 2019

Conversation

luandro
Copy link
Collaborator

@luandro luandro commented Sep 1, 2019

Hey @gmarcos87 and @nicopace! I'm dedicating this coming week for doing some Pirania work.

This is a first PR that fixes a major problem with the get_valid_macs function which was breaking everything; and also fixes some UI problems that I noticed in many phones.

Copy link
Contributor

@gmarcos87 gmarcos87 left a comment

Choose a reason for hiding this comment

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

Hi @luandro. It's a good thing that this repo is activated again!
There I published an observation about obtaining the IP. For future pull-request it is good that the changes are made in smaller and limited commits. Many of the changes made are stylistic, and it is hard to see the substantial differences.
It is a way to make the reviews, and further corrections simpler.

pirania-app/files/www/portal/js/voucher.js Show resolved Hide resolved
pirania-app/files/www/portal/js/content.js Outdated Show resolved Hide resolved
pirania-app/files/www/portal/js/voucher.js Outdated Show resolved Hide resolved
pirania-app/files/www/portal/js/voucher.js Outdated Show resolved Hide resolved
ubusError = true
})

})
Copy link
Member

Choose a reason for hiding this comment

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

for some reason this block is a little entangled in the viewer, ... can't tell easier what this part of the change is about.
as a suggestion, it easens the review if the changes are organized in commits that tell what the change is about... it makes the reviewer focus on the relation between the commit comment and the commit content, and any issue that could arrise from there...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe my linter?

pirania-app/files/www/portal/js/voucher.js Show resolved Hide resolved
pirania-app/files/www/portal/js/voucher.js Show resolved Hide resolved
@luandro
Copy link
Collaborator Author

luandro commented Sep 14, 2019

Are we waiting for something to merge this? @nicopace @gmarcos87

@nicopace nicopace merged commit c99c46b into master Sep 23, 2019
@nicopace
Copy link
Member

We will merge for now so @luandro can deal with the issue later without stopping the work.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants