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

Fixed Android form. Better tooltip component. #2234

Merged
merged 2 commits into from Nov 30, 2021
Merged

Conversation

JudahGabriel
Copy link
Contributor

@JudahGabriel JudahGabriel commented Nov 18, 2021

Fixes

Fixes #2219, fixes #2211, fixes #2249

PR Type

Bugfix
Refactoring (no functional changes, no api changes)

Describe the current behavior?

  • The Android form wasn't always setting the manifest URL properly.
  • The Android form had a great deal of duplicate code.
  • The hover tooltip didn't work well on scrollable modals.

Describe the new behavior?

  • The Android form sets the manifest URL correctly.
  • The Android form has been refactored to be more DRY, reduced by about 400 lines of code.
  • The hover tooltip has been reworked to use absolute position. I tested and verified it works correctly on the manifest options page, on the packaging modals, and on the publish page.
  • Adds an additional component, <info-circle-tooltip>, that draws a ❓ circle + hover to get a hover-tooltip.

@ghost
Copy link

ghost commented Nov 18, 2021

Thanks JudahGabriel for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost assigned davrous Nov 18, 2021
Copy link
Contributor

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

src/script/components/android-form.ts Show resolved Hide resolved
@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-field-047c1420f-2234.eastus2.azurestaticapps.net

Copy link
Contributor

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

👏 You fixed the issue(s)! Great work.

@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-field-047c1420f-2234.eastus2.azurestaticapps.net

@jgw96 jgw96 merged commit 50b9ed2 into main Nov 30, 2021
@jgw96 jgw96 deleted the android-manifest-url-fix branch November 30, 2021 20:34
@afrizal423
Copy link

Thank u 🙌

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.

Error generating android package Error generating android package Error generating android package
4 participants