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

Closes #1544 hide url controls on checkbox check event #1680

Closed
wants to merge 7 commits into from

Conversation

afarbman
Copy link

@afarbman afarbman commented Sep 17, 2020

Changes

Hide unnecessary URL controls when editing a pageview action

url-fix

Checklist

  • All querysets/queries filter by Team (if this PR affects any querysets/queries)
  • Backend tests (if this PR affects the backend)
  • Cypress E2E tests (if this PR affects the front and/or backend)

@Twixes
Copy link
Member

Twixes commented Sep 18, 2020

Ah, well, thank you, it's just that the URL text input box should disappear as well!

@jamesefhawkins
Copy link
Collaborator

@afarbman thank you so much for this. Email me (james at posthog.com) for something cool!

@afarbman
Copy link
Author

Ah, well, thank you, it's just that the URL text input box should disappear as well!

oh, I see. will take care of it.

Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

At the moment, the URL matching mode selector is shown initially, even though URL matching is disabled. The URL input box meanwhile is shown all the time, when it shouldn't be if URL matching is disabled.

@@ -20,6 +20,14 @@ let getSafeText = (el) => {
}

export class ActionStep extends Component {
isUrlChecked = (step) => {
return step?.selection?.indexOf('url') !== -1 ? true : false
Copy link
Member

Choose a reason for hiding this comment

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

A ternary with ? true : false is quite odd. !== already returns a boolean.

Copy link
Author

Choose a reason for hiding this comment

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

done in 24bd7e4

className="btn-group"
style={{
margin: isEditor ? '4px 0 0 8px' : '0 0 0 8px',
display: this.isUrlChecked(step) ? 'block' : 'none',
Copy link
Member

Choose a reason for hiding this comment

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

This would be better as a render condition (like so: this.isUrlChecked(step) & <div...) than dynamic display.

Copy link
Author

Choose a reason for hiding this comment

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

done in a4be717

@afarbman afarbman requested a review from Twixes September 20, 2020 22:27
@afarbman afarbman marked this pull request as ready for review September 20, 2020 22:39
@Twixes Twixes linked an issue Sep 25, 2020 that may be closed by this pull request
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Hm, unfortunately this is crashing the frontend for me when i go to http://localhost:8000/action and click "Page view".

@afarbman
Copy link
Author

afarbman commented Sep 27, 2020

Hm, unfortunately this is crashing the frontend for me when i go to http://localhost:8000/action and click "Page view".

@Twixes fixed in b1b6803

@afarbman afarbman force-pushed the 1544-fix-hide-url-controls branch from 2ef167a to b1b6803 Compare September 27, 2020 20:20
@afarbman afarbman requested a review from Twixes October 3, 2020 14:31
@Twixes
Copy link
Member

Twixes commented Oct 14, 2020

Ah, I'm still getting a crash when creating a new Page view Action, and URL input is by default shown on already existing Page view Actions even when URL is unchecked.
As we're making a general overhaul of the Actions page in #1841, I'll be closing this pull request – unfortunately this would cause conflicts. Sorry we didn't get this in, nevertheless we're tremendously thankful for your contribution @afarbman!

@Twixes Twixes closed this Oct 14, 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.

Hide unnecessary URL controls when editing a pageview action
3 participants