Skip to content

refactor tab modification by components - #6549

Merged
tymofij merged 1 commit into
feature/edxnotesfrom
tim/modify_tabs
Jan 12, 2015
Merged

refactor tab modification by components#6549
tymofij merged 1 commit into
feature/edxnotesfrom
tim/modify_tabs

Conversation

@tymofij

@tymofij tymofij commented Jan 12, 2015

Copy link
Copy Markdown
Contributor

@polesye please take a look

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd prefer to name it so is_advanced_module_present.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like _a_component_is_present and _component_value_true get the same arguments, but argument's names differ. I'd like to see the same names in both function. For example, components.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

let's settle on is_advanced_component_present - they are called components all over

@tymofij
tymofij force-pushed the tim/modify_tabs branch 2 times, most recently from 734ddeb to b157949 Compare January 12, 2015 12:42

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd prefer to name it so is_field_value_true or is_truthy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we really need to pass the request as an argument or we can pass request.json instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

is_field_value_true it is

@polesye

polesye commented Jan 12, 2015

Copy link
Copy Markdown
Contributor

👍 once tests pass and small comments will be addressed.

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.

2 participants