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.

Sign up for GitHub

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

Jump to bottom

Refactors Nature Sort By to use simpler radio button fieldset #687

Merged
merged 8 commits into from
Feb 16, 2022
Merged

Refactors Nature Sort By to use simpler radio button fieldset #687

merged 8 commits into from
Feb 16, 2022

Conversation

benjclark
Copy link
Contributor

@benjclark benjclark commented Feb 15, 2022 •
edited
Loading

This PR addresses the issues:

It does a complete rewrite of the component, to simplify it and improve it.

Component demo:

Mobile:
mobile

Desktop:
desktop

When applied to Nature's search results page:

Mobile:
Screenshot 2022-02-14 at 14 55 15

Desktop:
Screenshot 2022-02-14 at 14 55 27

amyhupe
amyhupe suggested changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@amyhupe amyhupe left a comment

Choose a reason for hiding this comment

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

Hi Ben,

Left you a couple of suggestions and questions. I may update the docs again to follow the new structure we're trying to use but I think it'll be simpler to do that as a separate PR once this has been merged.

Let me know if any questions.

toolkits/nature/packages/nature-sort-by/README.md Outdated Show resolved Hide resolved
toolkits/nature/packages/nature-sort-by/README.md Outdated Show resolved Hide resolved
toolkits/nature/packages/nature-sort-by/README.md Outdated
```

> **NOTE** The component require the use of the utility classes shown above
The component's HTML should be inserted inside of an HTML form in your application. See below [Template section](#template) for an example.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the anchor link here work on the Elements website? @sangitamane any idea?

Copy link
Contributor Author

@benjclark benjclark Feb 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks Amy. This approach for hash linking in README markdown files seems to be what we have done across all Elements components and brand contexts. So perhaps if we need to change that in any way we do it separately in another PR and we do across the entire Frontend Toolkits codebase?

image

toolkits/nature/packages/nature-sort-by/README.md Outdated Show resolved Hide resolved
amyhupe
amyhupe previously approved these changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@amyhupe amyhupe 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 to me now - thanks Ben 🚀

moddular
moddular approved these changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@moddular moddular left a comment

Choose a reason for hiding this comment

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

LGTM - thanks :)

@benjclark benjclark merged commit d9c6bed into master Feb 16, 2022
@benjclark benjclark deleted the refactor-nature-sort-by branch February 16, 2022 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moddular moddular moddular approved these changes

@amyhupe amyhupe amyhupe left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benjclark @moddular @amyhupe