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

fix: Update to use JSON tool definitions #1968

Merged
merged 4 commits into from
Oct 6, 2023
Merged

Conversation

karenjohn35
Copy link
Contributor

@karenjohn35 karenjohn35 commented Sep 22, 2023

The basics

The details

Resolves

Fixes #1263

Proposed Changes

Reason for Changes

Test Coverage

Documentation

Additional Information

Co-Authored-By: Cindy Ke <cindyko226@gmail.com>
@karenjohn35 karenjohn35 requested a review from a team as a code owner September 22, 2023 22:20
@karenjohn35 karenjohn35 requested review from BeksOmega and removed request for a team September 22, 2023 22:20
@google-cla
Copy link

google-cla bot commented Sep 22, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Contributor

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

This looks great! Just two little fixes then should be good to go =)

Thank you for your hard work on this!

examples/rtl-demo/index.html Outdated Show resolved Hide resolved
examples/rtl-demo/index.html Outdated Show resolved Hide resolved
karenjohn35 and others added 2 commits September 24, 2023 18:22
Co-authored-by: Beka Westberg <bwestberg@google.com>
Co-authored-by: Beka Westberg <bwestberg@google.com>
Copy link
Contributor

@debaraj-barua debaraj-barua Sep 25, 2023

Choose a reason for hiding this comment

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

@karenjohn35 Perhaps L390-L515 can be removed?

<xml xmlns="https://developers.google.com/blockly/xml" id="toolbox" style="display: none">
<category name="منطق" colour="%{BKY_LOGIC_HUE}">
<block type="controls_if"></block>
<block type="logic_compare"></block>
<block type="logic_operation"></block>
<block type="logic_negate"></block>
<block type="logic_boolean"></block>
<block type="logic_null"></block>
<block type="logic_ternary"></block>
</category>
<category name="الحلقات" colour="%{BKY_LOOPS_HUE}">
<block type="controls_repeat_ext">
<value name="TIMES">
<block type="math_number">
<field name="NUM">10</field>
</block>
</value>
</block>
<block type="controls_whileUntil"></block>
<block type="controls_for">
<value name="FROM">
<block type="math_number">
<field name="NUM">1</field>
</block>
</value>
<value name="TO">
<block type="math_number">
<field name="NUM">10</field>
</block>
</value>
<value name="BY">
<block type="math_number">
<field name="NUM">1</field>
</block>
</value>
</block>
<block type="controls_forEach"></block>
<block type="controls_flow_statements"></block>
</category>
<category name="رياضيات" colour="%{BKY_MATH_HUE}">
<block type="math_number">
<field name="NUM">123</field>
</block>
<block type="math_arithmetic"></block>
<block type="math_single"></block>
<block type="math_trig"></block>
<block type="math_constant"></block>
<block type="math_number_property"></block>
<block type="math_round"></block>
<block type="math_on_list"></block>
<block type="math_modulo"></block>
<block type="math_constrain">
<value name="LOW">
<block type="math_number">
<field name="NUM">1</field>
</block>
</value>
<value name="HIGH">
<block type="math_number">
<field name="NUM">100</field>
</block>
</value>
</block>
<block type="math_random_int">
<value name="FROM">
<block type="math_number">
<field name="NUM">1</field>
</block>
</value>
<value name="TO">
<block type="math_number">
<field name="NUM">100</field>
</block>
</value>
</block>
<block type="math_random_float"></block>
<block type="math_atan2"></block>
</category>
<category name="نص" colour="%{BKY_TEXTS_HUE}">
<block type="text"></block>
<block type="text_join"></block>
<block type="text_append">
<value name="TEXT">
<block type="text"></block>
</value>
</block>
<block type="text_length"></block>
<block type="text_isEmpty"></block>
<block type="text_indexOf"></block>
<block type="text_charAt"></block>
<block type="text_changeCase"></block>
<block type="text_trim"></block>
<block type="text_print"></block>
<block type="text_prompt_ext">
<value name="TEXT">
<block type="text"></block>
</value>
</block>
</category>
<category name="قوائم" colour="%{BKY_LISTS_HUE}">
<block type="lists_create_empty"></block>
<block type="lists_create_with"></block>
<block type="lists_repeat">
<value name="NUM">
<block type="math_number">
<field name="NUM">5</field>
</block>
</value>
</block>
<block type="lists_length"></block>
<block type="lists_isEmpty"></block>
<block type="lists_indexOf"></block>
<block type="lists_getIndex"></block>
<block type="lists_setIndex"></block>
</category>
<category name="لون" colour="%{BKY_COLOUR_HUE}">
<block type="colour_picker"></block>
<block type="colour_rgb"></block>
<block type="colour_blend"></block>
</category>
<sep></sep>
<category name="متغيرات" custom="VARIABLE" colour="%{BKY_VARIABLES_HUE}">
</category>
<category name="إجراءات" custom="PROCEDURE" colour="%{BKY_PROCEDURES_HUE}">
</category>
</xml>

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be great as well =) Thanks for the catch @debaraj-barua !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can I accept these changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

The UI here is a bit confusing, this isn't a suggested edit, just a link to the relevant lines of code!

@BeksOmega BeksOmega linked an issue Sep 29, 2023 that may be closed by this pull request
@BeksOmega
Copy link
Contributor

Heya @karenjohn35 Just wanted to check that you're still interested in working on this! Otherwise I'm going to close it on Friday 2023/10/06 so we don't have stale PRs =) Thank you again for your work on this!

@karenjohn35
Copy link
Contributor Author

Hi @BeksOmega! I think you can resolve this pull request after I implement the changes @debaraj-barua proposed. I am not entirely sure how I can do that. Would I have to create another pull request?

@BeksOmega
Copy link
Contributor

You would need to:

  1. Check out your OSD branch. git switch osd
  2. Delete the lines debaraj suggested
  3. Commit the changes. git commit -am "fix: delete xml"
  4. Push the changes to your remote. git push origin osd

Then I can get this merged!

@karenjohn35
Copy link
Contributor Author

Thanks, I just made that edit!

@BeksOmega BeksOmega merged commit 140a24f into google:osd Oct 6, 2023
@BeksOmega
Copy link
Contributor

Thank you so much for the work on this @karenjohn35 This looks great :D

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.

Update rtl-demo to use JSON toolbox definitions instead of xml
3 participants