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

Added validation for Google Analytics Campaign Tagging required fields (Issue29) #32

Merged
merged 10 commits into from
Jan 28, 2020

Conversation

petervnguyen99
Copy link
Contributor

@petervnguyen99 petervnguyen99 commented Jan 16, 2020

Pull Request For Issue #29
Added validation for Google Analytics Campaign Tagging required fields

Also simplified the display of the Go URL page by removing the shorten and show more buttons and displaying the Custom Alias and Google Analytics Campaign Tagging forms. With the Google Analytics Campaign Tagging form we added a checkbox which shows the form if checked and will reset the form if unchecked.

@jsturek
Copy link
Contributor

jsturek commented Jan 17, 2020

  • config.sample.php should not be deleted or edited (unless for new config ALL users of the repo will need). You are supposed to copy the file to config.inc.php and edit that file for your server's config. Please add the file back with no changes.

www/index.php Show resolved Hide resolved
www/templates/index.php Outdated Show resolved Hide resolved
src/lilURL.php Outdated Show resolved Hide resolved
src/lilURL.php Outdated Show resolved Hide resolved
www/templates/index.php Outdated Show resolved Hide resolved
www/templates/index.php Outdated Show resolved Hide resolved
@jsturek
Copy link
Contributor

jsturek commented Jan 17, 2020

If you want to make the google stuff available if javascript is disabled and work as desired when it is you can do the following:

  • Remove style from ga-tagging div so it displays by default
  • Hide by default all items we don't want to display when javascript is off by adding a class to identify these items, say ga-js-item and add style of display: none to hide.
    ** Hide google checkbox since not needed without javascript
    ** Hide the required google form items required spans since conditionally required
  • Add the following javascript to the google processing javascript:
// set up default display since javascript is available
    // show javascript dependent items
    var gaJSItems = document.getElementsByClassName('ga-js-item');
    for (var j=0; j<gaJSItems.length; j++) {
      gaJSItems[j].style.display = 'block';
    }
    // Hide google section by default
    gaSection.style.display = 'none';

Copy link
Contributor

@jsturek jsturek left a comment

Choose a reason for hiding this comment

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

See requested changes and suggestions

@jsturek
Copy link
Contributor

jsturek commented Jan 21, 2020

Changes look good. Thanks.

You should probably update the name/title of the PR to something descriptive and add a link to the issue in the description stating that the update addresses the issue. We don't have a formal convention for PRs for issues yet but it will be something like this.

@jsturek jsturek closed this Jan 21, 2020
Copy link
Contributor

@jsturek jsturek left a comment

Choose a reason for hiding this comment

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

Barring further changes I approve this PR.

@jsturek jsturek reopened this Jan 21, 2020
@jsturek jsturek self-requested a review January 21, 2020 19:24
Copy link
Contributor

@jsturek jsturek left a comment

Choose a reason for hiding this comment

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

Looks good.

www/templates/index.php Outdated Show resolved Hide resolved
@jsturek jsturek merged commit 90472b6 into unl:master Jan 28, 2020
@jsturek jsturek changed the title Issue29 Added validation for Google Analytics Campaign Tagging required fields (Issue29) Feb 10, 2020
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